[libcamera-devel] [PATCH v2 1/5] libcamera: delayed_controls: Add notion of priority write
paul.elder at ideasonboard.com
paul.elder at ideasonboard.com
Mon Mar 1 10:39:26 CET 2021
Hi Naush,
On Mon, Mar 01, 2021 at 08:57:46AM +0000, Naushir Patuck wrote:
> Hi Paul,
>
> On Mon, 1 Mar 2021 at 08:34, <paul.elder at ideasonboard.com> wrote:
>
> Hi Naush,
>
> On Tue, Feb 16, 2021 at 08:53:38AM +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 DelayedControls object 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.
>
> I'm confused (or it might be my lack of knowledge). You say that this
> "cannot be fixed by writing VBLANK before EXPOSURE" and then the
> workaround is to "write VBLANK separately from (and ahead of) any other
> controls"...?
>
>
> Sorry, this may have not come across in my wording above. If we write multiple
> controls in a set (via VIDIOC_S_EXT_CTRLS ioctl), then it does not matter
> if VBLANK is set before EXPOSURE, as since they are still part of the same
> set, EXPOSURE gets validated on the old VBLANK value.
>
> With this workaround, we explicitly move the VBLANK write out of the
> VIDIOC_S_EXT_CTRLS
> ioctl call with EXPOSURE in it, so this is not the case any more.
>
Oooh now I understand, thank you for the clarification.
I would prefer it if this was added to the changelog :)
>
> > To support this, a new struct DelayedControls::ControlParams is used in
> > the constructor to provide the control delay value as well as the
> > priority write flag.
>
> Other than my lack of understanding of the issue, I think the solution
> is fine, especially if it works. I'm just wondering if there will be any
> future need for multiple priority levels, or if just two (as this patch
> adds) is sufficient.
>
>
> I think two priority levels is enough for the likely usage I can forcee, but
> others may disagree?
Okay then. I suppose if we need more later we can add them then.
Looks good to me!
Reviewed-by: Paul Elder <paul.elder at ideasonboard.com>
> > Signed-off-by: Naushir Patuck <naush at raspberrypi.com>
> > ---
> > include/libcamera/internal/delayed_controls.h | 9 +++-
> > src/libcamera/delayed_controls.cpp | 54 +++++++++++++------
> > src/libcamera/pipeline/ipu3/ipu3.cpp | 8 +--
> > .../pipeline/raspberrypi/raspberrypi.cpp | 13 +++--
> > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 8 +--
> > test/delayed_contols.cpp | 20 +++----
> > 6 files changed, 68 insertions(+), 44 deletions(-)
> >
> > diff --git a/include/libcamera/internal/delayed_controls.h b/include/
> libcamera/internal/delayed_controls.h
> > index dc447a882514..564d9f2e2440 100644
> > --- a/include/libcamera/internal/delayed_controls.h
> > +++ b/include/libcamera/internal/delayed_controls.h
> > @@ -19,8 +19,13 @@ class V4L2Device;
> > class DelayedControls
> > {
> > public:
> > + struct ControlParams {
> > + unsigned int delay;
> > + bool priorityWrite;
> > + };
> > +
> > DelayedControls(V4L2Device *device,
> > - const std::unordered_map<uint32_t, unsigned int> &
> delays);
> > + const std::unordered_map<uint32_t, ControlParams> &
> controlParams);
> >
> > void reset();
> >
> > @@ -64,7 +69,7 @@ private:
> >
> > V4L2Device *device_;
> > /* \todo Evaluate if we should index on ControlId * or unsigned int
> */
> > - std::unordered_map<const ControlId *, unsigned int> delays_;
> > + std::unordered_map<const ControlId *, ControlParams>
> controlParams_;
> > unsigned int maxDelay_;
> >
> > bool running_;
> > diff --git a/src/libcamera/delayed_controls.cpp b/src/libcamera/
> delayed_controls.cpp
> > index ab1d40057c5f..3ed1dfebd035 100644
> > --- a/src/libcamera/delayed_controls.cpp
> > +++ b/src/libcamera/delayed_controls.cpp
> > @@ -40,15 +40,19 @@ LOG_DEFINE_CATEGORY(DelayedControls)
> > /**
> > * \brief Construct a DelayedControls instance
> > * \param[in] device The V4L2 device the controls have to be applied to
> > - * \param[in] delays Map of the numerical V4L2 control ids to their
> associated
> > - * delays (in frames)
> > + * \param[in] controlParams Map of the numerical V4L2 control ids to
> their
> > + * associated control parameters.
> > *
> > - * Only controls specified in \a delays are handled. If it's desired to
> mix
> > - * delayed controls and controls that take effect immediately the
> immediate
> > - * controls must be listed in the \a delays map with a delay value of 0.
> > + * The control parameters comprise of delays (in frames) and a priority
> write
> > + * flag. If this flag is set, the relevant control is written separately
> from,
> > + * ahead of the reset of the batched controls.
> > + *
> > + * Only controls specified in \a controlParams are handled. If it's
> desired to
> > + * mix delayed controls and controls that take effect immediately the
> immediate
> > + * controls must be listed in the \a controlParams map with a delay
> value of 0.
> > */
> > DelayedControls::DelayedControls(V4L2Device *device,
> > - const std::unordered_map<uint32_t,
> unsigned int> &delays)
> > + const std::unordered_map<uint32_t,
> ControlParams> &controlParams)
> > : device_(device), maxDelay_(0)
> > {
> > const ControlInfoMap &controls = device_->controls();
> > @@ -57,12 +61,12 @@ DelayedControls::DelayedControls(V4L2Device *device,
> > * Create a map of control ids to delays for controls exposed by
> the
> > * device.
> > */
> > - for (auto const &delay : delays) {
> > - auto it = controls.find(delay.first);
> > + for (auto const ¶m : controlParams) {
> > + auto it = controls.find(param.first);
> > if (it == controls.end()) {
> > LOG(DelayedControls, Error)
> > << "Delay request for control id "
> > - << utils::hex(delay.first)
> > + << utils::hex(param.first)
> > << " but control is not exposed by device "
> > << device_->deviceNode();
> > continue;
> > @@ -70,13 +74,14 @@ DelayedControls::DelayedControls(V4L2Device *device,
> >
> > const ControlId *id = it->first;
> >
> > - delays_[id] = delay.second;
> > + controlParams_[id] = param.second;
> >
> > LOG(DelayedControls, Debug)
> > - << "Set a delay of " << delays_[id]
> > + << "Set a delay of " << controlParams_[id].delay
> > + << " and priority write flag " << controlParams_
> [id].priorityWrite
> > << " for " << id->name();
> >
> > - maxDelay_ = std::max(maxDelay_, delays_[id]);
> > + maxDelay_ = std::max(maxDelay_, controlParams_[id].delay);
> > }
> >
> > reset();
> > @@ -97,8 +102,8 @@ void DelayedControls::reset()
> >
> > /* Retrieve control as reported by the device. */
> > std::vector<uint32_t> ids;
> > - for (auto const &delay : delays_)
> > - ids.push_back(delay.first->id());
> > + for (auto const ¶m : controlParams_)
> > + ids.push_back(param.first->id());
> >
> > ControlList controls = device_->getControls(ids);
> >
> > @@ -140,7 +145,7 @@ bool DelayedControls::push(const ControlList &
> controls)
> >
> > const ControlId *id = it->second;
> >
> > - if (delays_.find(id) == delays_.end())
> > + if (controlParams_.find(id) == controlParams_.end())
> > return false;
> >
> > Info &info = values_[id][queueCount_];
> > @@ -220,12 +225,27 @@ void DelayedControls::applyControls(uint32_t
> sequence)
> > ControlList out(device_->controls());
> > for (const auto &ctrl : values_) {
> > const ControlId *id = ctrl.first;
> > - unsigned int delayDiff = maxDelay_ - delays_[id];
> > + unsigned int delayDiff = maxDelay_ - controlParams_
> [id].delay;
> > unsigned int index = std::max<int>(0, writeCount_ -
> delayDiff);
> > const Info &info = ctrl.second[index];
> >
> > if (info.updated) {
> > - out.set(id->id(), info);
> > + if (controlParams_[id].priorityWrite) {
> > + /*
> > + * This control must be written now, it
> could
> > + * affect validity of the other controls.
> > + */
> > + ControlList priority(device_->controls());
> > + priority.set(id->id(), info);
> > + device_->setControls(&priority);
> > + } else {
> > + /*
> > + * Batch up the list of controls and write
> them
> > + * at the end of the function.
> > + */
> > + out.set(id->id(), info);
> > + }
> > +
> > LOG(DelayedControls, Debug)
> > << "Setting " << id->name()
> > << " to " << info.toString()
> > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/
> pipeline/ipu3/ipu3.cpp
> > index 61f7bf43ea08..eda07d915f0c 100644
> > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > @@ -988,14 +988,14 @@ int PipelineHandlerIPU3::registerCameras()
> > * a sensor database. For now use generic values taken from
> > * the Raspberry Pi and listed as 'generic values'.
> > */
> > - std::unordered_map<uint32_t, unsigned int> delays = {
> > - { V4L2_CID_ANALOGUE_GAIN, 1 },
> > - { V4L2_CID_EXPOSURE, 2 },
> > + std::unordered_map<uint32_t,
> DelayedControls::ControlParams> params = {
> > + { V4L2_CID_ANALOGUE_GAIN, { 1, false } },
> > + { V4L2_CID_EXPOSURE, { 2, false } },
> > };
> >
> > data->delayedCtrls_ =
> > std::make_unique<DelayedControls>(cio2->sensor()->
> device(),
> > - delays);
> > + params);
> > data->cio2_.frameStart().connect(data->delayedCtrls_.get(),
> > &
> DelayedControls::applyControls);
> >
> > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/
> libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > index b0bb12ab0f37..bdb439021c46 100644
> > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > @@ -1273,16 +1273,15 @@ int RPiCameraData::configureIPA(const
> CameraConfiguration *config)
> > if (result.operation & RPi::IPA_RESULT_SENSOR_PARAMS) {
> > /*
> > * Setup our delayed control writer with the sensor default
> > - * gain and exposure delays.
> > + * gain and exposure delays. Mark VBLANK for priority
> write.
> > */
> > - std::unordered_map<uint32_t, unsigned int> delays = {
> > - { V4L2_CID_ANALOGUE_GAIN, result.data[resultIdx++]
> },
> > - { V4L2_CID_EXPOSURE, result.data[resultIdx++] },
> > - { V4L2_CID_VBLANK, result.data[resultIdx++] }
> > + std::unordered_map<uint32_t,
> DelayedControls::ControlParams> params = {
> > + { V4L2_CID_ANALOGUE_GAIN, { result.data
> [resultIdx++], false } },
> > + { V4L2_CID_EXPOSURE, { result.data[resultIdx++],
> false } },
> > + { V4L2_CID_VBLANK, { result.data[resultIdx++], true
> } }
> > };
> >
> > - delayedCtrls_ = std::make_unique<DelayedControls>(unicam_
> [Unicam::Image].dev(), delays);
> > -
> > + delayedCtrls_ = std::make_unique<DelayedControls>(unicam_
> [Unicam::Image].dev(), params);
> > sensorMetadata_ = result.data[resultIdx++];
> > }
> >
> > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/
> pipeline/rkisp1/rkisp1.cpp
> > index e85979a719b7..ea9b1e2b9fcb 100644
> > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > @@ -942,14 +942,14 @@ int PipelineHandlerRkISP1::createCamera(MediaEntity
> *sensor)
> > * a sensor database. For now use generic values taken from
> > * the Raspberry Pi and listed as generic values.
> > */
> > - std::unordered_map<uint32_t, unsigned int> delays = {
> > - { V4L2_CID_ANALOGUE_GAIN, 1 },
> > - { V4L2_CID_EXPOSURE, 2 },
> > + std::unordered_map<uint32_t, DelayedControls::ControlParams> params
> = {
> > + { V4L2_CID_ANALOGUE_GAIN, { 1, false } },
> > + { V4L2_CID_EXPOSURE, { 2, false } },
> > };
> >
> > data->delayedCtrls_ =
> > std::make_unique<DelayedControls>(data->sensor_->device(),
> > - delays);
> > + params);
> > isp_->frameStart.connect(data->delayedCtrls_.get(),
> > &DelayedControls::applyControls);
> >
> > diff --git a/test/delayed_contols.cpp b/test/delayed_contols.cpp
> > index 50169b12e566..3855eb18ecd4 100644
> > --- a/test/delayed_contols.cpp
> > +++ b/test/delayed_contols.cpp
> > @@ -72,8 +72,8 @@ protected:
> >
> > int singleControlNoDelay()
> > {
> > - std::unordered_map<uint32_t, unsigned int> delays = {
> > - { V4L2_CID_BRIGHTNESS, 0 },
> > + std::unordered_map<uint32_t,
> DelayedControls::ControlParams> delays = {
> > + { V4L2_CID_BRIGHTNESS, { 0, false } },
> > };
> > std::unique_ptr<DelayedControls> delayed =
> > std::make_unique<DelayedControls>(dev_.get(),
> delays);
> > @@ -109,8 +109,8 @@ protected:
> >
> > int singleControlWithDelay()
> > {
> > - std::unordered_map<uint32_t, unsigned int> delays = {
> > - { V4L2_CID_BRIGHTNESS, 1 },
> > + std::unordered_map<uint32_t,
> DelayedControls::ControlParams> delays = {
> > + { V4L2_CID_BRIGHTNESS, { 1, false } },
> > };
> > std::unique_ptr<DelayedControls> delayed =
> > std::make_unique<DelayedControls>(dev_.get(),
> delays);
> > @@ -150,9 +150,9 @@ protected:
> >
> > int dualControlsWithDelay(uint32_t startOffset)
> > {
> > - std::unordered_map<uint32_t, unsigned int> delays = {
> > - { V4L2_CID_BRIGHTNESS, 1 },
> > - { V4L2_CID_CONTRAST, 2 },
> > + std::unordered_map<uint32_t,
> DelayedControls::ControlParams> delays = {
> > + { V4L2_CID_BRIGHTNESS, { 1, false } },
> > + { V4L2_CID_CONTRAST, { 2, false } },
> > };
> > std::unique_ptr<DelayedControls> delayed =
> > std::make_unique<DelayedControls>(dev_.get(),
> delays);
> > @@ -197,9 +197,9 @@ protected:
> >
> > int dualControlsMultiQueue()
> > {
> > - std::unordered_map<uint32_t, unsigned int> delays = {
> > - { V4L2_CID_BRIGHTNESS, 1 },
> > - { V4L2_CID_CONTRAST, 2 },
> > + std::unordered_map<uint32_t,
> DelayedControls::ControlParams> delays = {
> > + { V4L2_CID_BRIGHTNESS, { 1, false } },
> > + { V4L2_CID_CONTRAST, { 2, false } }
> > };
> > std::unique_ptr<DelayedControls> delayed =
> > std::make_unique<DelayedControls>(dev_.get(),
> delays);
> > --
> > 2.25.1
> >
> > _______________________________________________
> > libcamera-devel mailing list
> > libcamera-devel at lists.libcamera.org
> > https://lists.libcamera.org/listinfo/libcamera-devel
>
More information about the libcamera-devel
mailing list