[libcamera-devel] [PATCH v4 5/5] pipeline: raspberrypi: Add notion of priority write to StaggeredCtrl
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Thu Feb 4 21:22:01 CET 2021
Hi Naush,
Thank you for the patch.
On Fri, Jan 29, 2021 at 11:16:16AM +0000, Naushir Patuck wrote:
> If an exposure time change adjusts the vblanking limits, and we write
> both VBLANK and EXPOSURE controls as a set, the latter may fail if the
> value is outside of the limits calculated by the old VBLANK value. This
> is a limitation in V4L2 and cannot be fixed by writing VBLANK before
> EXPOSURE.
>
> The workaround here is to have the StaggeredCtrl mark the
> VBLANK control as "priority write", which then write VBLANK separately
> from (and ahead of) any other controls. This way, the sensor driver will
> update the EXPOSURE control with new limits before the new values is
> presented, and will thus be seen as valid.
>
> In addition to this, the following changes have also been made to
> the module:
>
> - The initializer list passed into init() now uses a structure type
> instead of a std::pair.
> - Use unsigned int to store control delays to avoid unnecessary casts.
>
> StaggeredCtrl is due to be deprecated and replaced by DelayedCtrl, so
> this change serves more a working proof-of-concept on the workaround,
> and not much care has been taken to provide a nice new API for applying
> this "priority write" flag to the control. A similar workaround must be
> available to DelayedCtrl eventually.
>
> Signed-off-by: Naushir Patuck <naush at raspberrypi.com>
> Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>
I'll drop this patch for now, as the StaggeredCtrl implementation has
been replaced.
> ---
> src/ipa/raspberrypi/raspberrypi.cpp | 5 ++-
> .../pipeline/raspberrypi/raspberrypi.cpp | 12 ++++--
> .../pipeline/raspberrypi/staggered_ctrl.cpp | 41 +++++++++++++------
> .../pipeline/raspberrypi/staggered_ctrl.h | 17 ++++++--
> 4 files changed, 54 insertions(+), 21 deletions(-)
>
> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp
> index 8c0e699184f6..2ad7b7dabb3e 100644
> --- a/src/ipa/raspberrypi/raspberrypi.cpp
> +++ b/src/ipa/raspberrypi/raspberrypi.cpp
> @@ -1038,8 +1038,9 @@ void IPARPi::applyAGC(const struct AgcStatus *agcStatus, ControlList &ctrls)
>
> /*
> * Due to the behavior of V4L2, the current value of VBLANK could clip the
> - * exposure time without us knowing. The next time though this function should
> - * clip exposure correctly.
> + * exposure time without us knowing. We get around this by ensuring the
> + * staggered write component submits VBLANK separately from, and before the
> + * EXPOSURE control.
> */
> ctrls.set(V4L2_CID_VBLANK, vblanking);
> ctrls.set(V4L2_CID_EXPOSURE, exposureLines);
> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> index 524cc960dd37..2118f2e72486 100644
> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> @@ -1229,12 +1229,18 @@ int RPiCameraData::configureIPA(const CameraConfiguration *config)
> /*
> * Setup our staggered control writer with the sensor default
> * gain and exposure delays.
> + *
> + * VBLANK must be flagged as "priority write" to allow it to
> + * be set ahead of (and separate from) all other controls that
> + * are batched together. This is needed so that any update to the
> + * EXPOSURE control will be validated based on the new VBLANK
> + * control value.
> */
> if (!staggeredCtrl_) {
> staggeredCtrl_.init(unicam_[Unicam::Image].dev(),
> - { { V4L2_CID_ANALOGUE_GAIN, result.data[resultIdx++] },
> - { V4L2_CID_EXPOSURE, result.data[resultIdx++] },
> - { V4L2_CID_VBLANK, result.data[resultIdx++] } });
> + { { V4L2_CID_ANALOGUE_GAIN, result.data[resultIdx++], false },
> + { V4L2_CID_EXPOSURE, result.data[resultIdx++], false },
> + { V4L2_CID_VBLANK, result.data[resultIdx++], true } });
> sensorMetadata_ = result.data[resultIdx++];
> }
> }
> diff --git a/src/libcamera/pipeline/raspberrypi/staggered_ctrl.cpp b/src/libcamera/pipeline/raspberrypi/staggered_ctrl.cpp
> index 62605c0fceee..498cd65b4cb6 100644
> --- a/src/libcamera/pipeline/raspberrypi/staggered_ctrl.cpp
> +++ b/src/libcamera/pipeline/raspberrypi/staggered_ctrl.cpp
> @@ -22,21 +22,23 @@ LOG_DEFINE_CATEGORY(RPI_S_W)
> namespace RPi {
>
> void StaggeredCtrl::init(V4L2VideoDevice *dev,
> - std::initializer_list<std::pair<const uint32_t, uint8_t>> delayList)
> + std::initializer_list<CtrlInitParams> ctrlList)
> {
> std::lock_guard<std::mutex> lock(lock_);
>
> dev_ = dev;
> - delay_ = delayList;
> + ctrlParams_.clear();
> ctrl_.clear();
>
> - /* Find the largest delay across all controls. */
> maxDelay_ = 0;
> - for (auto const &p : delay_) {
> + for (auto const &c : ctrlList) {
> LOG(RPI_S_W, Info) << "Init ctrl "
> - << utils::hex(p.first) << " with delay "
> - << static_cast<int>(p.second);
> - maxDelay_ = std::max(maxDelay_, p.second);
> + << utils::hex(c.id) << " with delay "
> + << static_cast<int>(c.delay);
> +
> + ctrlParams_[c.id] = { c.delay, c.priorityWrite };
> + /* Find the largest delay across all controls. */
> + maxDelay_ = std::max(maxDelay_, c.delay);
> }
>
> init_ = true;
> @@ -67,7 +69,7 @@ bool StaggeredCtrl::set(uint32_t ctrl, int32_t value)
> std::lock_guard<std::mutex> lock(lock_);
>
> /* Can we find this ctrl as one that is registered? */
> - if (delay_.find(ctrl) == delay_.end())
> + if (ctrlParams_.find(ctrl) == ctrlParams_.end())
> return false;
>
> ctrl_[ctrl][setCount_].value = value;
> @@ -82,7 +84,7 @@ bool StaggeredCtrl::set(std::initializer_list<std::pair<const uint32_t, int32_t>
>
> for (auto const &p : ctrlList) {
> /* Can we find this ctrl? */
> - if (delay_.find(p.first) == delay_.end())
> + if (ctrlParams_.find(p.first) == ctrlParams_.end())
> return false;
>
> ctrl_[p.first][setCount_] = CtrlInfo(p.second);
> @@ -97,7 +99,7 @@ bool StaggeredCtrl::set(const ControlList &controls)
>
> for (auto const &p : controls) {
> /* Can we find this ctrl? */
> - if (delay_.find(p.first) == delay_.end())
> + if (ctrlParams_.find(p.first) == ctrlParams_.end())
> return false;
>
> ctrl_[p.first][setCount_] = CtrlInfo(p.second.get<int32_t>());
> @@ -117,12 +119,25 @@ int StaggeredCtrl::write()
> ControlList controls(dev_->controls());
>
> for (auto &p : ctrl_) {
> - int delayDiff = maxDelay_ - delay_[p.first];
> + int delayDiff = maxDelay_ - ctrlParams_[p.first].delay;
> int index = std::max<int>(0, setCount_ - delayDiff);
>
> if (p.second[index].updated) {
> - /* We need to write this value out. */
> - controls.set(p.first, p.second[index].value);
> + if (ctrlParams_[p.first].priorityWrite) {
> + /*
> + * This control must be written now, it could
> + * affect validity of the other controls.
> + */
> + ControlList immediate(dev_->controls());
> + immediate.set(p.first, p.second[index].value);
> + dev_->setControls(&immediate);
> + } else {
> + /*
> + * Batch up the list of controls and write them
> + * at the end of the function.
> + */
> + controls.set(p.first, p.second[index].value);
> + }
> p.second[index].updated = false;
> LOG(RPI_S_W, Debug) << "Writing ctrl "
> << utils::hex(p.first) << " to "
> diff --git a/src/libcamera/pipeline/raspberrypi/staggered_ctrl.h b/src/libcamera/pipeline/raspberrypi/staggered_ctrl.h
> index 382fa31a6853..637629c0d9a8 100644
> --- a/src/libcamera/pipeline/raspberrypi/staggered_ctrl.h
> +++ b/src/libcamera/pipeline/raspberrypi/staggered_ctrl.h
> @@ -23,6 +23,12 @@ namespace RPi {
> class StaggeredCtrl
> {
> public:
> + struct CtrlInitParams {
> + unsigned int id;
> + unsigned int delay;
> + bool priorityWrite;
> + };
> +
> StaggeredCtrl()
> : init_(false), setCount_(0), getCount_(0), maxDelay_(0)
> {
> @@ -34,7 +40,7 @@ public:
> }
>
> void init(V4L2VideoDevice *dev,
> - std::initializer_list<std::pair<const uint32_t, uint8_t>> delayList);
> + std::initializer_list<CtrlInitParams> ctrlList);
> void reset();
>
> void get(std::unordered_map<uint32_t, int32_t> &ctrl, uint8_t offset = 0);
> @@ -79,12 +85,17 @@ private:
> }
> };
>
> + struct CtrlParams {
> + unsigned int delay;
> + bool priorityWrite;
> + };
> +
> bool init_;
> uint32_t setCount_;
> uint32_t getCount_;
> - uint8_t maxDelay_;
> + unsigned int maxDelay_;
> V4L2VideoDevice *dev_;
> - std::unordered_map<uint32_t, uint8_t> delay_;
> + std::unordered_map<uint32_t, CtrlParams> ctrlParams_;
> std::unordered_map<uint32_t, CircularArray> ctrl_;
> std::mutex lock_;
> };
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list