<div dir="ltr"><div dir="ltr">Hi Kieran,<div><br></div><div>Thank you for your review feedback.</div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Fri, 12 Mar 2021 at 14:00, Kieran Bingham <<a href="mailto:kieran.bingham@ideasonboard.com">kieran.bingham@ideasonboard.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><br>
<br>
On 12/03/2021 13:25, Kieran Bingham wrote:<br>
> Hi Naush,<br>
> <br>
> On 09/03/2021 09:54, Jean-Michel Hautbois wrote:<br>
>> Hi Naushir,<br>
>><br>
>> Thanks for the patch !<br>
>><br>
>> On 04/03/2021 09:17, Naushir Patuck wrote:<br>
>>> If an exposure time change adjusts the vblanking limits, and we set both<br>
>>> VBLANK and EXPOSURE controls through the VIDIOC_S_EXT_CTRLS ioctl, the<br>
>>> latter may fail if the value is outside of the limits calculated by the<br>
>>> old VBLANK value. This is a limitation in V4L2 and cannot be fixed by<br>
>>> setting VBLANK before EXPOSURE in a single VIDIOC_S_EXT_CTRLS ioctl.<br>
>>><br>
>>> The workaround here is to have the DelayedControls object mark the<br>
>>> VBLANK control as "priority write", which then write VBLANK separately<br>
>>> from (and ahead of) any other controls. This way, the sensor driver will<br>
>>> update the EXPOSURE control with new limits before the new values is<br>
>>> presented, and will thus be seen as valid.<br>
>>><br>
>>> To support this, a new struct DelayedControls::ControlParams is used in<br>
>>> the constructor to provide the control delay value as well as the<br>
>>> priority write flag.<br>
>>><br>
>>> Signed-off-by: Naushir Patuck <<a href="mailto:naush@raspberrypi.com" target="_blank">naush@raspberrypi.com</a>><br>
>>> Tested-by: David Plowman <<a href="mailto:david.plowman@raspberrypi.com" target="_blank">david.plowman@raspberrypi.com</a>><br>
>>> Reviewed-by: Paul Elder <<a href="mailto:paul.elder@ideasonboard.com" target="_blank">paul.elder@ideasonboard.com</a>><br>
>> Tested-by: Jean-Michel Hautbois <<a href="mailto:jeanmichel.hautbois@ideasonboard.com" target="_blank">jeanmichel.hautbois@ideasonboard.com</a>><br>
>><br>
>>> ---<br>
>>> include/libcamera/internal/delayed_controls.h | 9 +++-<br>
>>> src/libcamera/delayed_controls.cpp | 54 +++++++++++++------<br>
>>> src/libcamera/pipeline/ipu3/ipu3.cpp | 8 +--<br>
>>> .../pipeline/raspberrypi/raspberrypi.cpp | 13 +++--<br>
>>> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 8 +--<br>
>>> test/delayed_contols.cpp | 20 +++----<br>
>>> 6 files changed, 68 insertions(+), 44 deletions(-)<br>
>>><br>
>>> diff --git a/include/libcamera/internal/delayed_controls.h b/include/libcamera/internal/delayed_controls.h<br>
>>> index dc447a882514..564d9f2e2440 100644<br>
>>> --- a/include/libcamera/internal/delayed_controls.h<br>
>>> +++ b/include/libcamera/internal/delayed_controls.h<br>
>>> @@ -19,8 +19,13 @@ class V4L2Device;<br>
>>> class DelayedControls<br>
>>> {<br>
>>> public:<br>
>>> + struct ControlParams {<br>
>>> + unsigned int delay;<br>
>>> + bool priorityWrite;<br>
>>> + };<br>
>>> +<br>
>>> DelayedControls(V4L2Device *device,<br>
>>> - const std::unordered_map<uint32_t, unsigned int> &delays);<br>
>>> + const std::unordered_map<uint32_t, ControlParams> &controlParams);<br>
>>> <br>
>>> void reset();<br>
>>> <br>
>>> @@ -64,7 +69,7 @@ private:<br>
>>> <br>
>>> V4L2Device *device_;<br>
>>> /* \todo Evaluate if we should index on ControlId * or unsigned int */<br>
>>> - std::unordered_map<const ControlId *, unsigned int> delays_;<br>
>>> + std::unordered_map<const ControlId *, ControlParams> controlParams_;<br>
>>> unsigned int maxDelay_;<br>
>>> <br>
>>> bool running_;<br>
>>> diff --git a/src/libcamera/delayed_controls.cpp b/src/libcamera/delayed_controls.cpp<br>
>>> index ab1d40057c5f..3ed1dfebd035 100644<br>
>>> --- a/src/libcamera/delayed_controls.cpp<br>
>>> +++ b/src/libcamera/delayed_controls.cpp<br>
>>> @@ -40,15 +40,19 @@ LOG_DEFINE_CATEGORY(DelayedControls)<br>
>>> /**<br>
>>> * \brief Construct a DelayedControls instance<br>
>>> * \param[in] device The V4L2 device the controls have to be applied to<br>
>>> - * \param[in] delays Map of the numerical V4L2 control ids to their associated<br>
>>> - * delays (in frames)<br>
>>> + * \param[in] controlParams Map of the numerical V4L2 control ids to their<br>
>>> + * associated control parameters.<br>
>>> *<br>
>>> - * Only controls specified in \a delays are handled. If it's desired to mix<br>
>>> - * delayed controls and controls that take effect immediately the immediate<br>
>>> - * controls must be listed in the \a delays map with a delay value of 0.<br>
>>> + * The control parameters comprise of delays (in frames) and a priority write<br>
>>> + * flag. If this flag is set, the relevant control is written separately from,<br>
>>> + * ahead of the reset of the batched controls.<br>
> <br>
> minor:<br>
> <br>
> 'and ahead of' ?<br>
> <br>
> could be fixed while applying if there's nothing else major:<br>
<br>
<br>
Could you confirm if this is correct to say 'and ahead of the reset of'<br>
or if it was supposed to be 'and ahead of the rest of' ...<br></blockquote><div><br></div><div>'separately from, and ahead of the rest of` should be the correct term here :)</div><div><br></div><div>Happy for you to fix while applying if it is convenient for you.</div><div><br></div><div>Regards,</div><div>Naush</div><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
<br>
> <br>
> Reviewed-by: Kieran Bingham <<a href="mailto:kieran.bingham@ideasonboard.com" target="_blank">kieran.bingham@ideasonboard.com</a>><br>
> <br>
> <br>
> <br>
>>> + *<br>
>>> + * Only controls specified in \a controlParams are handled. If it's desired to<br>
>>> + * mix delayed controls and controls that take effect immediately the immediate<br>
>>> + * controls must be listed in the \a controlParams map with a delay value of 0.<br>
>>> */<br>
>>> DelayedControls::DelayedControls(V4L2Device *device,<br>
>>> - const std::unordered_map<uint32_t, unsigned int> &delays)<br>
>>> + const std::unordered_map<uint32_t, ControlParams> &controlParams)<br>
>>> : device_(device), maxDelay_(0)<br>
>>> {<br>
>>> const ControlInfoMap &controls = device_->controls();<br>
>>> @@ -57,12 +61,12 @@ DelayedControls::DelayedControls(V4L2Device *device,<br>
>>> * Create a map of control ids to delays for controls exposed by the<br>
>>> * device.<br>
>>> */<br>
>>> - for (auto const &delay : delays) {<br>
>>> - auto it = controls.find(delay.first);<br>
>>> + for (auto const ¶m : controlParams) {<br>
>>> + auto it = controls.find(param.first);<br>
>>> if (it == controls.end()) {<br>
>>> LOG(DelayedControls, Error)<br>
>>> << "Delay request for control id "<br>
>>> - << utils::hex(delay.first)<br>
>>> + << utils::hex(param.first)<br>
>>> << " but control is not exposed by device "<br>
>>> << device_->deviceNode();<br>
>>> continue;<br>
>>> @@ -70,13 +74,14 @@ DelayedControls::DelayedControls(V4L2Device *device,<br>
>>> <br>
>>> const ControlId *id = it->first;<br>
>>> <br>
>>> - delays_[id] = delay.second;<br>
>>> + controlParams_[id] = param.second;<br>
>>> <br>
>>> LOG(DelayedControls, Debug)<br>
>>> - << "Set a delay of " << delays_[id]<br>
>>> + << "Set a delay of " << controlParams_[id].delay<br>
>>> + << " and priority write flag " << controlParams_[id].priorityWrite<br>
>>> << " for " << id->name();<br>
>>> <br>
>>> - maxDelay_ = std::max(maxDelay_, delays_[id]);<br>
>>> + maxDelay_ = std::max(maxDelay_, controlParams_[id].delay);<br>
>>> }<br>
>>> <br>
>>> reset();<br>
>>> @@ -97,8 +102,8 @@ void DelayedControls::reset()<br>
>>> <br>
>>> /* Retrieve control as reported by the device. */<br>
>>> std::vector<uint32_t> ids;<br>
>>> - for (auto const &delay : delays_)<br>
>>> - ids.push_back(delay.first->id());<br>
>>> + for (auto const ¶m : controlParams_)<br>
>>> + ids.push_back(param.first->id());<br>
>>> <br>
>>> ControlList controls = device_->getControls(ids);<br>
>>> <br>
>>> @@ -140,7 +145,7 @@ bool DelayedControls::push(const ControlList &controls)<br>
>>> <br>
>>> const ControlId *id = it->second;<br>
>>> <br>
>>> - if (delays_.find(id) == delays_.end())<br>
>>> + if (controlParams_.find(id) == controlParams_.end())<br>
>>> return false;<br>
>>> <br>
>>> Info &info = values_[id][queueCount_];<br>
>>> @@ -220,12 +225,27 @@ void DelayedControls::applyControls(uint32_t sequence)<br>
>>> ControlList out(device_->controls());<br>
>>> for (const auto &ctrl : values_) {<br>
>>> const ControlId *id = ctrl.first;<br>
>>> - unsigned int delayDiff = maxDelay_ - delays_[id];<br>
>>> + unsigned int delayDiff = maxDelay_ - controlParams_[id].delay;<br>
>>> unsigned int index = std::max<int>(0, writeCount_ - delayDiff);<br>
>>> const Info &info = ctrl.second[index];<br>
>>> <br>
>>> if (info.updated) {<br>
>>> - out.set(id->id(), info);<br>
>>> + if (controlParams_[id].priorityWrite) {<br>
>>> + /*<br>
>>> + * This control must be written now, it could<br>
>>> + * affect validity of the other controls.<br>
>>> + */<br>
>>> + ControlList priority(device_->controls());<br>
>>> + priority.set(id->id(), info);<br>
>>> + device_->setControls(&priority);<br>
>>> + } else {<br>
>>> + /*<br>
>>> + * Batch up the list of controls and write them<br>
>>> + * at the end of the function.<br>
>>> + */<br>
>>> + out.set(id->id(), info);<br>
>>> + }<br>
>>> +<br>
>>> LOG(DelayedControls, Debug)<br>
>>> << "Setting " << id->name()<br>
>>> << " to " << info.toString()<br>
>>> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp<br>
>>> index 3e6b88af4188..ac92f066a07e 100644<br>
>>> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp<br>
>>> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp<br>
>>> @@ -963,14 +963,14 @@ int PipelineHandlerIPU3::registerCameras()<br>
>>> * a sensor database. For now use generic values taken from<br>
>>> * the Raspberry Pi and listed as 'generic values'.<br>
>>> */<br>
>>> - std::unordered_map<uint32_t, unsigned int> delays = {<br>
>>> - { V4L2_CID_ANALOGUE_GAIN, 1 },<br>
>>> - { V4L2_CID_EXPOSURE, 2 },<br>
>>> + std::unordered_map<uint32_t, DelayedControls::ControlParams> params = {<br>
>>> + { V4L2_CID_ANALOGUE_GAIN, { 1, false } },<br>
>>> + { V4L2_CID_EXPOSURE, { 2, false } },<br>
>>> };<br>
>>> <br>
>>> data->delayedCtrls_ =<br>
>>> std::make_unique<DelayedControls>(cio2->sensor()->device(),<br>
>>> - delays);<br>
>>> + params);<br>
>>> data->cio2_.frameStart().connect(data->delayedCtrls_.get(),<br>
>>> &DelayedControls::applyControls);<br>
>>> <br>
>>> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp<br>
>>> index a60415d93705..ba74ace183db 100644<br>
>>> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp<br>
>>> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp<br>
>>> @@ -1244,16 +1244,15 @@ int RPiCameraData::configureIPA(const CameraConfiguration *config)<br>
>>> if (result.params & ipa::RPi::ConfigSensorParams) {<br>
>>> /*<br>
>>> * Setup our delayed control writer with the sensor default<br>
>>> - * gain and exposure delays.<br>
>>> + * gain and exposure delays. Mark VBLANK for priority write.<br>
>>> */<br>
>>> - std::unordered_map<uint32_t, unsigned int> delays = {<br>
>>> - { V4L2_CID_ANALOGUE_GAIN, result.sensorConfig.gainDelay },<br>
>>> - { V4L2_CID_EXPOSURE, result.sensorConfig.exposureDelay },<br>
>>> - { V4L2_CID_VBLANK, result.sensorConfig.vblank }<br>
>>> + std::unordered_map<uint32_t, DelayedControls::ControlParams> params = {<br>
>>> + { V4L2_CID_ANALOGUE_GAIN, { result.sensorConfig.gainDelay, false } },<br>
>>> + { V4L2_CID_EXPOSURE, { result.sensorConfig.exposureDelay, false } },<br>
>>> + { V4L2_CID_VBLANK, { result.sensorConfig.vblank, true } }<br>
<br>
Applying has a merge conflict on master here.<br>
<br>
Setting this as<br>
{ V4L2_CID_VBLANK, { result.sensorConfig.vblankDelay, true } }<br>
<br>
<br>
<br>
>>> };<br>
>>> <br>
>>> - delayedCtrls_ = std::make_unique<DelayedControls>(unicam_[Unicam::Image].dev(), delays);<br>
>>> -<br>
>>> + delayedCtrls_ = std::make_unique<DelayedControls>(unicam_[Unicam::Image].dev(), params);<br>
>>> sensorMetadata_ = result.sensorConfig.sensorMetadata;<br>
>>> }<br>
>>> <br>
>>> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp<br>
>>> index a794501a9c8d..17c0f9751cd3 100644<br>
>>> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp<br>
>>> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp<br>
>>> @@ -938,14 +938,14 @@ int PipelineHandlerRkISP1::createCamera(MediaEntity *sensor)<br>
>>> * a sensor database. For now use generic values taken from<br>
>>> * the Raspberry Pi and listed as generic values.<br>
>>> */<br>
>>> - std::unordered_map<uint32_t, unsigned int> delays = {<br>
>>> - { V4L2_CID_ANALOGUE_GAIN, 1 },<br>
>>> - { V4L2_CID_EXPOSURE, 2 },<br>
>>> + std::unordered_map<uint32_t, DelayedControls::ControlParams> params = {<br>
>>> + { V4L2_CID_ANALOGUE_GAIN, { 1, false } },<br>
>>> + { V4L2_CID_EXPOSURE, { 2, false } },<br>
>>> };<br>
>>> <br>
>>> data->delayedCtrls_ =<br>
>>> std::make_unique<DelayedControls>(data->sensor_->device(),<br>
>>> - delays);<br>
>>> + params);<br>
>>> isp_->frameStart.connect(data->delayedCtrls_.get(),<br>
>>> &DelayedControls::applyControls);<br>
>>> <br>
>>> diff --git a/test/delayed_contols.cpp b/test/delayed_contols.cpp<br>
>>> index 50169b12e566..3855eb18ecd4 100644<br>
>>> --- a/test/delayed_contols.cpp<br>
>>> +++ b/test/delayed_contols.cpp<br>
>>> @@ -72,8 +72,8 @@ protected:<br>
>>> <br>
>>> int singleControlNoDelay()<br>
>>> {<br>
>>> - std::unordered_map<uint32_t, unsigned int> delays = {<br>
>>> - { V4L2_CID_BRIGHTNESS, 0 },<br>
>>> + std::unordered_map<uint32_t, DelayedControls::ControlParams> delays = {<br>
>>> + { V4L2_CID_BRIGHTNESS, { 0, false } },<br>
>>> };<br>
>>> std::unique_ptr<DelayedControls> delayed =<br>
>>> std::make_unique<DelayedControls>(dev_.get(), delays);<br>
>>> @@ -109,8 +109,8 @@ protected:<br>
>>> <br>
>>> int singleControlWithDelay()<br>
>>> {<br>
>>> - std::unordered_map<uint32_t, unsigned int> delays = {<br>
>>> - { V4L2_CID_BRIGHTNESS, 1 },<br>
>>> + std::unordered_map<uint32_t, DelayedControls::ControlParams> delays = {<br>
>>> + { V4L2_CID_BRIGHTNESS, { 1, false } },<br>
>>> };<br>
>>> std::unique_ptr<DelayedControls> delayed =<br>
>>> std::make_unique<DelayedControls>(dev_.get(), delays);<br>
>>> @@ -150,9 +150,9 @@ protected:<br>
>>> <br>
>>> int dualControlsWithDelay(uint32_t startOffset)<br>
>>> {<br>
>>> - std::unordered_map<uint32_t, unsigned int> delays = {<br>
>>> - { V4L2_CID_BRIGHTNESS, 1 },<br>
>>> - { V4L2_CID_CONTRAST, 2 },<br>
>>> + std::unordered_map<uint32_t, DelayedControls::ControlParams> delays = {<br>
>>> + { V4L2_CID_BRIGHTNESS, { 1, false } },<br>
>>> + { V4L2_CID_CONTRAST, { 2, false } },<br>
>>> };<br>
>>> std::unique_ptr<DelayedControls> delayed =<br>
>>> std::make_unique<DelayedControls>(dev_.get(), delays);<br>
>>> @@ -197,9 +197,9 @@ protected:<br>
>>> <br>
>>> int dualControlsMultiQueue()<br>
>>> {<br>
>>> - std::unordered_map<uint32_t, unsigned int> delays = {<br>
>>> - { V4L2_CID_BRIGHTNESS, 1 },<br>
>>> - { V4L2_CID_CONTRAST, 2 },<br>
>>> + std::unordered_map<uint32_t, DelayedControls::ControlParams> delays = {<br>
>>> + { V4L2_CID_BRIGHTNESS, { 1, false } },<br>
>>> + { V4L2_CID_CONTRAST, { 2, false } }<br>
>>> };<br>
>>> std::unique_ptr<DelayedControls> delayed =<br>
>>> std::make_unique<DelayedControls>(dev_.get(), delays);<br>
>>><br>
>> _______________________________________________<br>
>> libcamera-devel mailing list<br>
>> <a href="mailto:libcamera-devel@lists.libcamera.org" target="_blank">libcamera-devel@lists.libcamera.org</a><br>
>> <a href="https://lists.libcamera.org/listinfo/libcamera-devel" rel="noreferrer" target="_blank">https://lists.libcamera.org/listinfo/libcamera-devel</a><br>
>><br>
> <br>
<br>
-- <br>
Regards<br>
--<br>
Kieran<br>
</blockquote></div></div>