<div dir="ltr"><div dir="ltr">Hi Kieran,<div><br></div><div>Thank you for the review!</div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Wed, 26 Oct 2022 at 17:38, 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">Quoting Naushir Patuck via libcamera-devel (2022-10-19 10:01:02)<br>
> Allow the caller to provide a cookie value to DelayedControls::reset and<br>
> DelayedControls::push. This cookie value is returned from DelayedControls::get<br>
> for the frame that has the control values applied to it.<br>
> <br>
> The cookie value is useful in tracking when a set of controls has been applied<br>
> to a frame. In a subsequent commit, it will be used by the Raspberry Pi IPA to<br>
> track the IPA context used when setting the control values.<br>
> <br>
> Signed-off-by: Naushir Patuck <<a href="mailto:naush@raspberrypi.com" target="_blank">naush@raspberrypi.com</a>><br>
> Reviewed-by: David Plowman <<a href="mailto:david.plowman@raspberrypi.com" target="_blank">david.plowman@raspberrypi.com</a>><br>
> Tested-by: David Plowman <<a href="mailto:david.plowman@raspberrypi.com" target="_blank">david.plowman@raspberrypi.com</a>><br>
> ---<br>
> include/libcamera/internal/delayed_controls.h | 8 +++++---<br>
> src/libcamera/delayed_controls.cpp | 20 ++++++++++++-------<br>
> src/libcamera/pipeline/ipu3/ipu3.cpp | 3 ++-<br>
> .../pipeline/raspberrypi/raspberrypi.cpp | 2 +-<br>
> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 3 ++-<br>
> test/delayed_controls.cpp | 8 ++++----<br>
> 6 files changed, 27 insertions(+), 17 deletions(-)<br>
> <br>
> diff --git a/include/libcamera/internal/delayed_controls.h b/include/libcamera/internal/delayed_controls.h<br>
> index de8026e3e4f0..c7e79150afb8 100644<br>
> --- a/include/libcamera/internal/delayed_controls.h<br>
> +++ b/include/libcamera/internal/delayed_controls.h<br>
> @@ -9,6 +9,7 @@<br>
> <br>
> #include <stdint.h><br>
> #include <unordered_map><br>
> +#include <utility><br>
> <br>
> #include <libcamera/controls.h><br>
> <br>
> @@ -27,10 +28,10 @@ public:<br>
> DelayedControls(V4L2Device *device,<br>
> const std::unordered_map<uint32_t, ControlParams> &controlParams);<br>
> <br>
> - void reset();<br>
> + void reset(unsigned int cookie = 0);<br>
> <br>
> - bool push(const ControlList &controls);<br>
> - ControlList get(uint32_t sequence);<br>
> + bool push(const ControlList &controls, unsigned int cookie = 0);<br>
<br>
The part that worries me here is that we /require/ always getting the<br>
cookie, but not setting it...<br>
<br>
I would feel safer if this wasn't defaulted to 0...<br></blockquote><div><br></div><div>Sure, I'll remove the default values from push() and reset(), and make the below<br>suggested changes to the ipu3 and rkisp pipeline handlers.<br><br>Regards,<br>Naush<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>
> + std::pair<ControlList, unsigned int> get(uint32_t sequence);<br>
> <br>
> void applyControls(uint32_t sequence);<br>
> <br>
> @@ -77,6 +78,7 @@ private:<br>
> uint32_t writeCount_;<br>
> /* \todo Evaluate if we should index on ControlId * or unsigned int */<br>
> std::unordered_map<const ControlId *, RingBuffer<Info>> values_;<br>
> + RingBuffer<unsigned int> cookies_;<br>
> };<br>
> <br>
> } /* namespace libcamera */<br>
> diff --git a/src/libcamera/delayed_controls.cpp b/src/libcamera/delayed_controls.cpp<br>
> index 777441e8222a..3e7f60f9c421 100644<br>
> --- a/src/libcamera/delayed_controls.cpp<br>
> +++ b/src/libcamera/delayed_controls.cpp<br>
> @@ -109,14 +109,16 @@ DelayedControls::DelayedControls(V4L2Device *device,<br>
> <br>
> /**<br>
> * \brief Reset state machine<br>
> + * \param[in] cookie User supplied reset cookie value<br>
> *<br>
> * Resets the state machine to a starting position based on control values<br>
> * retrieved from the device.<br>
> */<br>
> -void DelayedControls::reset()<br>
> +void DelayedControls::reset(unsigned int cookie)<br>
> {<br>
> queueCount_ = 1;<br>
> writeCount_ = 0;<br>
> + cookies_[0] = cookie;<br>
> <br>
> /* Retrieve control as reported by the device. */<br>
> std::vector<uint32_t> ids;<br>
> @@ -140,13 +142,15 @@ void DelayedControls::reset()<br>
> /**<br>
> * \brief Push a set of controls on the queue<br>
> * \param[in] controls List of controls to add to the device queue<br>
> + * \param[in] cookie User supplied cookie value for \a controls<br>
> *<br>
> * Push a set of controls to the control queue. This increases the control queue<br>
> - * depth by one.<br>
> + * depth by one. The \a cookie value will be subsequently returned from<br>
> + * \a get() for the frame with all controls applied.<br>
> *<br>
> * \returns true if \a controls are accepted, or false otherwise<br>
> */<br>
> -bool DelayedControls::push(const ControlList &controls)<br>
> +bool DelayedControls::push(const ControlList &controls, const unsigned int cookie)<br>
> {<br>
> /* Copy state from previous frame. */<br>
> for (auto &ctrl : values_) {<br>
> @@ -180,6 +184,7 @@ bool DelayedControls::push(const ControlList &controls)<br>
> << " at index " << queueCount_;<br>
> }<br>
> <br>
> + cookies_[queueCount_] = cookie;<br>
> queueCount_++;<br>
> <br>
> return true;<br>
> @@ -198,9 +203,10 @@ bool DelayedControls::push(const ControlList &controls)<br>
> * push(). The max history from the current sequence number that yields valid<br>
> * values are thus 16 minus number of controls pushed.<br>
> *<br>
> - * \return The controls at \a sequence number<br>
> + * \return The controls at \a sequence number and associated user supplied<br>
> + * cookie value.<br>
> */<br>
> -ControlList DelayedControls::get(uint32_t sequence)<br>
> +std::pair<ControlList, unsigned int> DelayedControls::get(uint32_t sequence)<br>
> {<br>
> unsigned int index = std::max<int>(0, sequence - maxDelay_);<br>
> <br>
> @@ -217,7 +223,7 @@ ControlList DelayedControls::get(uint32_t sequence)<br>
> << " at index " << index;<br>
> }<br>
> <br>
> - return out;<br>
> + return { out, cookies_[index] };<br>
> }<br>
> <br>
> /**<br>
> @@ -276,7 +282,7 @@ void DelayedControls::applyControls(uint32_t sequence)<br>
> while (writeCount_ > queueCount_) {<br>
> LOG(DelayedControls, Debug)<br>
> << "Queue is empty, auto queue no-op.";<br>
> - push({});<br>
> + push({}, cookies_[queueCount_ - 1]);<br>
> }<br>
> <br>
> device_->setControls(&out);<br>
> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp<br>
> index 3b892d9671c5..bf612089fdcb 100644<br>
> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp<br>
> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp<br>
> @@ -1389,7 +1389,8 @@ void IPU3CameraData::cio2BufferReady(FrameBuffer *buffer)<br>
> request->metadata().set(controls::SensorTimestamp,<br>
> buffer->metadata().timestamp);<br>
> <br>
> - info->effectiveSensorControls = delayedCtrls_->get(buffer->metadata().sequence);<br>
> + auto [controls, cookie] = delayedCtrls_->get(buffer->metadata().sequence);<br>
> + info->effectiveSensorControls = std::move(controls);<br>
<br>
Which means this cookie is always zero.<br>
<br>
Could you also update the push call to set the cookie?<br>
<br>
That looks like it happens at<br>
<br>
void IPU3CameraData::setSensorControls([[maybe_unused]] unsigned int id,<br>
const ControlList &sensorControls,<br>
const ControlList &lensControls)<br>
{<br>
delayedCtrls_->push(sensorControls);<br>
...<br>
<br>
Where the 'id' is actually the request sequence number in the current<br>
implementation.<br>
<br>
So we can remove [[maybe_unused]] from unsigned int id, and pass it to push().<br>
<br>
Ultimately, this then means that the cookie is the request sequence<br>
number of the request which is supposed to reflect these controls. Which<br>
is a nice way of tieing these together.<br>
<br>
<br>
> if (request->findBuffer(&rawStream_))<br>
> pipe()->completeBuffer(request, buffer);<br>
> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp<br>
> index 343f8cb2c7ed..23f2460190f4 100644<br>
> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp<br>
> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp<br>
> @@ -1867,7 +1867,7 @@ void RPiCameraData::unicamBufferDequeue(FrameBuffer *buffer)<br>
> * Lookup the sensor controls used for this frame sequence from<br>
> * DelayedControl and queue them along with the frame buffer.<br>
> */<br>
> - ControlList ctrl = delayedCtrls_->get(buffer->metadata().sequence);<br>
> + auto [ctrl, cookie] = delayedCtrls_->get(buffer->metadata().sequence);<br>
> /*<br>
> * Add the frame timestamp to the ControlList for the IPA to use<br>
> * as it does not receive the FrameBuffer object.<br>
> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp<br>
> index 455ee2a0a711..20049d089472 100644<br>
> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp<br>
> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp<br>
> @@ -1191,8 +1191,9 @@ void PipelineHandlerRkISP1::statReady(FrameBuffer *buffer)<br>
> if (data->frame_ <= buffer->metadata().sequence)<br>
> data->frame_ = buffer->metadata().sequence + 1;<br>
> <br>
> + auto [controls, cookie] = data->delayedCtrls_->get(buffer->metadata().sequence);<br>
<br>
Same for rkisp1:<br>
<br>
void RkISP1CameraData::setSensorControls([[maybe_unused]] unsigned int frame,<br>
const ControlList &sensorControls)<br>
{<br>
delayedCtrls_->push(sensorControls);<br>
}<br>
<br>
... 'frame' should be a suitable/expected cookie here.<br>
<br>
<br>
> data->ipa_->processStatsBuffer(info->frame, info->statBuffer->cookie(),<br>
> - data->delayedCtrls_->get(buffer->metadata().sequence));<br>
> + controls);<br>
> }<br>
> <br>
> REGISTER_PIPELINE_HANDLER(PipelineHandlerRkISP1)<br>
> diff --git a/test/delayed_controls.cpp b/test/delayed_controls.cpp<br>
> index a8ce9828d73d..322c545998b2 100644<br>
> --- a/test/delayed_controls.cpp<br>
> +++ b/test/delayed_controls.cpp<br>
<br>
And in here, testing locally - then the pushes here need a cookie - and<br>
'i' seems suitable.<br>
<br>
With that,<br>
<br>
<br>
Reviewed-by: Kieran Bingham <<a href="mailto:kieran.bingham@ideasonboard.com" target="_blank">kieran.bingham@ideasonboard.com</a>><br>
<br>
> @@ -96,7 +96,7 @@ protected:<br>
> <br>
> delayed->applyControls(i);<br>
> <br>
> - ControlList result = delayed->get(i);<br>
> + auto [result, cookie] = delayed->get(i);<br>
> int32_t brightness = result.get(V4L2_CID_BRIGHTNESS).get<int32_t>();<br>
> if (brightness != value) {<br>
> cerr << "Failed single control without delay"<br>
> @@ -138,7 +138,7 @@ protected:<br>
> <br>
> delayed->applyControls(i);<br>
> <br>
> - ControlList result = delayed->get(i);<br>
> + auto [result, cookie] = delayed->get(i);<br>
> int32_t brightness = result.get(V4L2_CID_BRIGHTNESS).get<int32_t>();<br>
> if (brightness != expected) {<br>
> cerr << "Failed single control with delay"<br>
> @@ -187,7 +187,7 @@ protected:<br>
> <br>
> delayed->applyControls(i);<br>
> <br>
> - ControlList result = delayed->get(i);<br>
> + auto [result, cookie] = delayed->get(i);<br>
> int32_t brightness = result.get(V4L2_CID_BRIGHTNESS).get<int32_t>();<br>
> int32_t contrast = result.get(V4L2_CID_CONTRAST).get<int32_t>();<br>
> if (brightness != expected || contrast != expected + 1) {<br>
> @@ -247,7 +247,7 @@ protected:<br>
> <br>
> delayed->applyControls(i);<br>
> <br>
> - ControlList result = delayed->get(i);<br>
> + auto [result, cookie] = delayed->get(i);<br>
> <br>
> int32_t brightness = result.get(V4L2_CID_BRIGHTNESS).get<int32_t>();<br>
> int32_t contrast = result.get(V4L2_CID_CONTRAST).get<int32_t>();<br>
> -- <br>
> 2.25.1<br>
><br>
</blockquote></div></div>