<div dir="ltr"><div dir="ltr">Hi Niklas,<div><br></div><div>Thank you for your patch.</div><div><br></div></div><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Wed, 28 Oct 2020 at 01:01, Niklas Söderlund <<a href="mailto:niklas.soderlund@ragnatech.se">niklas.soderlund@ragnatech.se</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">Use the libcamera core helper DelayedControls instead of the local<br>
StaggeredCtrl. The new helper is modeled after the StaggeredCtrl<br>
implementation and behaves the same.<br>
<br>
Signed-off-by: Niklas Söderlund <<a href="mailto:niklas.soderlund@ragnatech.se" target="_blank">niklas.soderlund@ragnatech.se</a>><br>
---<br>
.../pipeline/raspberrypi/raspberrypi.cpp | 44 +++++++++----------<br>
1 file changed, 20 insertions(+), 24 deletions(-)<br>
<br>
diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp<br>
index a48013d8c27b98eb..4087985f1e66c940 100644<br>
--- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp<br>
+++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp<br>
@@ -25,6 +25,7 @@<br>
<br>
#include "libcamera/internal/bayer_format.h"<br>
#include "libcamera/internal/camera_sensor.h"<br>
+#include "libcamera/internal/delayed_controls.h"<br>
#include "libcamera/internal/device_enumerator.h"<br>
#include "libcamera/internal/ipa_manager.h"<br>
#include "libcamera/internal/media_device.h"<br>
@@ -35,7 +36,6 @@<br>
<br>
#include "dma_heaps.h"<br>
#include "rpi_stream.h"<br>
-#include "staggered_ctrl.h"<br>
<br>
namespace libcamera {<br>
<br>
@@ -169,8 +169,7 @@ public:<br>
RPi::DmaHeap dmaHeap_;<br>
FileDescriptor lsTable_;<br>
<br>
- RPi::StaggeredCtrl staggeredCtrl_;<br>
- uint32_t expectedSequence_;<br>
+ std::unique_ptr<DelayedControls> delayedCtrls_;<br>
bool sensorMetadata_;<br>
<br>
/*<br>
@@ -773,10 +772,7 @@ int PipelineHandlerRPi::start(Camera *camera)<br>
* starting. First check that the staggered ctrl has been initialised<br>
* by configure().<br>
*/<br>
- ASSERT(data->staggeredCtrl_);<br>
- data->staggeredCtrl_.reset();<br>
- data->staggeredCtrl_.write();<br>
- data->expectedSequence_ = 0;<br>
+ data->delayedCtrls_->reset();<br>
<br>
data->state_ = RPiCameraData::State::Idle;<br>
<br>
@@ -1107,7 +1103,7 @@ void RPiCameraData::frameStarted(uint32_t sequence)<br>
LOG(RPI, Debug) << "frame start " << sequence;<br>
<br>
/* Write any controls for the next frame as soon as we can. */<br>
- staggeredCtrl_.write();<br>
+ delayedCtrls_->frameStart(sequence);<br>
}<br>
<br>
int RPiCameraData::loadIPA()<br>
@@ -1185,18 +1181,22 @@ int RPiCameraData::configureIPA(const CameraConfiguration *config)<br>
* Setup our staggered control writer with the sensor default<br>
* gain and exposure delays.<br>
*/<br>
- if (!staggeredCtrl_) {<br>
- staggeredCtrl_.init(unicam_[Unicam::Image].dev(),<br>
- { { V4L2_CID_ANALOGUE_GAIN, result.data[resultIdx++] },<br>
- { V4L2_CID_EXPOSURE, result.data[resultIdx++] } });<br>
+<br>
+ if (!delayedCtrls_) {<br>
+ std::unordered_map<uint32_t, unsigned int> delays = {<br>
+ { V4L2_CID_ANALOGUE_GAIN, result.data[resultIdx++] },<br>
+ { V4L2_CID_EXPOSURE, result.data[resultIdx++] }<br>
+ };<br>
+<br>
+ delayedCtrls_ = std::make_unique<DelayedControls>(unicam_[Unicam::Image].dev(), delays);<br>
+<br>
sensorMetadata_ = result.data[resultIdx++];<br>
}<br>
}<br>
<br>
if (result.operation & RPi::IPA_CONFIG_SENSOR) {<br>
- const ControlList &ctrls = result.controls[0];<br>
- if (!staggeredCtrl_.set(ctrls))<br>
- LOG(RPI, Error) << "V4L2 staggered set failed";<br>
+ ControlList ctrls = result.controls[0];<br>
+ delayedCtrls_->reset(&ctrls);<br>
}<br>
<br>
if (result.operation & RPi::IPA_CONFIG_DROP_FRAMES) {<br>
@@ -1230,8 +1230,8 @@ void RPiCameraData::queueFrameAction([[maybe_unused]] unsigned int frame,<br>
switch (action.operation) {<br>
case RPi::IPA_ACTION_V4L2_SET_STAGGERED: {<br>
const ControlList &controls = action.controls[0];<br>
- if (!staggeredCtrl_.set(controls))<br>
- LOG(RPI, Error) << "V4L2 staggered set failed";<br>
+ if (!delayedCtrls_->push(controls))<br>
+ LOG(RPI, Error) << "V4L2 delay set failed";<br>
goto done;<br>
}<br>
<br>
@@ -1335,11 +1335,7 @@ void RPiCameraData::unicamBufferDequeue(FrameBuffer *buffer)<br>
} else {<br>
embeddedQueue_.push(buffer);<br>
<br>
- std::unordered_map<uint32_t, int32_t> ctrl;<br>
- int offset = buffer->metadata().sequence - expectedSequence_;<br>
- staggeredCtrl_.get(ctrl, offset);<br>
-<br>
- expectedSequence_ = buffer->metadata().sequence + 1;<br>
+ ControlList ctrl = delayedCtrls_->get(buffer->metadata().sequence);<br></blockquote><div><br></div><div>Just to confirm my understanding is correct, insead of tracking
expectedSequence_ and calculating offset to account for any number of frame drops, we use the buffer->metadata().sequence directly to do the same thing? If so, this is a great simplification.</div><div><br></div><div>Regards,</div><div>Naush</div><div> </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>
* Sensor metadata is unavailable, so put the expected ctrl<br>
@@ -1352,8 +1348,8 @@ void RPiCameraData::unicamBufferDequeue(FrameBuffer *buffer)<br>
PROT_READ | PROT_WRITE,<br>
MAP_SHARED,<br>
fb.planes()[0].fd.fd(), 0));<br>
- mem[0] = ctrl[V4L2_CID_EXPOSURE];<br>
- mem[1] = ctrl[V4L2_CID_ANALOGUE_GAIN];<br>
+ mem[0] = ctrl.get(V4L2_CID_EXPOSURE).get<int32_t>();<br>
+ mem[1] = ctrl.get(V4L2_CID_ANALOGUE_GAIN).get<int32_t>();<br>
munmap(mem, fb.planes()[0].length);<br>
}<br>
}<br>
-- <br>
2.29.1<br>
<br>
</blockquote></div></div>