[libcamera-devel] [PATCH v2 1/5] libcamera: delayed_controls: Add notion of priority write
Naushir Patuck
naush at raspberrypi.com
Mon Mar 1 09:57:46 CET 2021
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.
>
> > 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?
Regards,
Naush
>
>
> Paul
>
> > 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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20210301/00c26e19/attachment-0001.htm>
More information about the libcamera-devel
mailing list