[libcamera-devel] [PATCH v4 1/7] libcamera: delayed_controls: Add notion of priority write
Kieran Bingham
kieran.bingham at ideasonboard.com
Fri Mar 12 15:00:02 CET 2021
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' ...
>
> 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
More information about the libcamera-devel
mailing list