[libcamera-devel] [PATCH v4 1/7] libcamera: delayed_controls: Add notion of priority write
Naushir Patuck
naush at raspberrypi.com
Fri Mar 12 15:04:27 CET 2021
Hi Kieran,
Thank you for your review feedback.
On Fri, 12 Mar 2021 at 14:00, Kieran Bingham <
kieran.bingham at ideasonboard.com> wrote:
>
>
> On 12/03/2021 13:25, Kieran Bingham wrote:
> > Hi Naush,
> >
> > On 09/03/2021 09:54, Jean-Michel Hautbois wrote:
> >> Hi Naushir,
> >>
> >> Thanks for the patch !
> >>
> >> On 04/03/2021 09:17, Naushir Patuck wrote:
> >>> If an exposure time change adjusts the vblanking limits, and we set
> both
> >>> VBLANK and EXPOSURE controls through the VIDIOC_S_EXT_CTRLS ioctl, 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
> >>> setting VBLANK before EXPOSURE in a single VIDIOC_S_EXT_CTRLS ioctl.
> >>>
> >>> 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.
> >>>
> >>> 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.
> >>>
> >>> Signed-off-by: Naushir Patuck <naush at raspberrypi.com>
> >>> Tested-by: David Plowman <david.plowman at raspberrypi.com>
> >>> Reviewed-by: Paul Elder <paul.elder at ideasonboard.com>
> >> Tested-by: Jean-Michel Hautbois <jeanmichel.hautbois at ideasonboard.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.
> >
> > minor:
> >
> > 'and ahead of' ?
> >
> > could be fixed while applying if there's nothing else major:
>
>
> Could you confirm if this is correct to say 'and ahead of the reset of'
> or if it was supposed to be 'and ahead of the rest of' ...
>
'separately from, and ahead of the rest of` should be the correct term here
:)
Happy for you to fix while applying if it is convenient for you.
Regards,
Naush
>
>
> >
> > Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> >
> >
> >
> >>> + *
> >>> + * 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 3e6b88af4188..ac92f066a07e 100644
> >>> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> >>> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> >>> @@ -963,14 +963,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 a60415d93705..ba74ace183db 100644
> >>> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> >>> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> >>> @@ -1244,16 +1244,15 @@ int RPiCameraData::configureIPA(const
> CameraConfiguration *config)
> >>> if (result.params & ipa::RPi::ConfigSensorParams) {
> >>> /*
> >>> * 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.sensorConfig.gainDelay },
> >>> - { V4L2_CID_EXPOSURE,
> result.sensorConfig.exposureDelay },
> >>> - { V4L2_CID_VBLANK, result.sensorConfig.vblank }
> >>> + std::unordered_map<uint32_t,
> DelayedControls::ControlParams> params = {
> >>> + { V4L2_CID_ANALOGUE_GAIN, {
> result.sensorConfig.gainDelay, false } },
> >>> + { V4L2_CID_EXPOSURE, {
> result.sensorConfig.exposureDelay, false } },
> >>> + { V4L2_CID_VBLANK, { result.sensorConfig.vblank,
> true } }
>
> Applying has a merge conflict on master here.
>
> Setting this as
> { V4L2_CID_VBLANK, { result.sensorConfig.vblankDelay, true } }
>
>
>
> >>> };
> >>>
> >>> - delayedCtrls_ =
> std::make_unique<DelayedControls>(unicam_[Unicam::Image].dev(), delays);
> >>> -
> >>> + delayedCtrls_ =
> std::make_unique<DelayedControls>(unicam_[Unicam::Image].dev(), params);
> >>> sensorMetadata_ = result.sensorConfig.sensorMetadata;
> >>> }
> >>>
> >>> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> >>> index a794501a9c8d..17c0f9751cd3 100644
> >>> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> >>> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> >>> @@ -938,14 +938,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);
> >>>
> >> _______________________________________________
> >> libcamera-devel mailing list
> >> libcamera-devel at lists.libcamera.org
> >> https://lists.libcamera.org/listinfo/libcamera-devel
> >>
> >
>
> --
> Regards
> --
> Kieran
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20210312/e6f5e7b2/attachment-0001.htm>
More information about the libcamera-devel
mailing list