<div dir="ltr"><div dir="ltr">Hi Niklas,<div><br></div><div>Thank you for the changes. I will go through all the patches with review comment in due course, but wanted to raise a critical point below:</div></div><br><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" target="_blank">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">Expose the optional DelayedControls interface to pipeline handlers. If<br>
used by a pipeline generic delays are used. In the future the delay<br>
values should be fetched to match the specific sensor module, either<br>
from a new kernel interface or worst case a sensors database.<br>
<br>
Signed-off-by: Niklas Söderlund <<a href="mailto:niklas.soderlund@ragnatech.se" target="_blank">niklas.soderlund@ragnatech.se</a>><br>
---<br>
include/libcamera/internal/camera_sensor.h | 5 ++++<br>
src/libcamera/camera_sensor.cpp | 31 ++++++++++++++++++++++<br>
2 files changed, 36 insertions(+)<br>
<br>
diff --git a/include/libcamera/internal/camera_sensor.h b/include/libcamera/internal/camera_sensor.h<br>
index b9eba89f00fba882..6be1cfd49134c534 100644<br>
--- a/include/libcamera/internal/camera_sensor.h<br>
+++ b/include/libcamera/internal/camera_sensor.h<br>
@@ -14,6 +14,7 @@<br>
#include <libcamera/controls.h><br>
#include <libcamera/geometry.h><br>
<br>
+#include "libcamera/internal/delayed_controls.h"<br>
#include "libcamera/internal/formats.h"<br>
#include "libcamera/internal/log.h"<br>
#include "libcamera/internal/v4l2_subdevice.h"<br>
@@ -61,6 +62,8 @@ public:<br>
ControlList getControls(const std::vector<uint32_t> &ids);<br>
int setControls(ControlList *ctrls);<br>
<br>
+ DelayedControls *delayedContols();<br>
+<br>
const ControlList &properties() const { return properties_; }<br>
int sensorInfo(CameraSensorInfo *info) const;<br>
<br>
@@ -83,6 +86,8 @@ private:<br>
std::vector<Size> sizes_;<br>
<br>
ControlList properties_;<br>
+<br>
+ std::unique_ptr<DelayedControls> delayedControls_;<br>
};<br>
<br>
} /* namespace libcamera */<br>
diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp<br>
index 935de528c4963453..6c92c97f4cc2547a 100644<br>
--- a/src/libcamera/camera_sensor.cpp<br>
+++ b/src/libcamera/camera_sensor.cpp<br>
@@ -493,6 +493,37 @@ int CameraSensor::setControls(ControlList *ctrls)<br>
return subdev_->setControls(ctrls);<br>
}<br>
<br>
+/**<br>
+ * \brief Get the sensors delayed controls interface<br>
+ *<br>
+ * Access the sensors delayed controls interface. This interface aids pipelines<br>
+ * keeping tack of controls that needs time to take effect. The interface is<br>
+ * optional to use and does not interact with setControls() and getControls()<br>
+ * that operates directly on the sensor device.<br>
+ *<br>
+ * \sa DelayedControls<br>
+ * \return The delayed controls interface<br>
+ */<br>
+DelayedControls *CameraSensor::delayedContols()<br>
+{<br>
+ if (!delayedControls_) {<br>
+ /*<br>
+ * \todo Read dealy values from the sensor itself or from a<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>
+ };<br></blockquote><div><br></div><div>This will break the OV5647 sensor usage with Raspberry Pi without referring back to a sensor database .
OV5647
has a delay of 2 for gain and exposure, so will not work with the above hard-coded settings.</div><div><br></div><div>In a more general sense, I wonder if initialising the DelayedControl here is the right thing to do? I have some changes waiting to be submitted for review that add framerate control to the Raspberry Pi stack. In this change, I've added a VBLANK control to the staggered writer. There are also some changes to control very long exposures on the imx477 that go through the staggered writer. In these cases, I would have to add values to the map above, but other pipeline handlers would never need to use them. This may be a bit wasteful, I don't know... Would it make more sense for the pipeline handlers to initialise the DelayedControls as they need to?</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>
+ delayedControls_ =<br>
+ std::make_unique<DelayedControls>(subdev_.get(), delays);<br>
+ }<br>
+<br>
+ return delayedControls_.get();<br>
+}<br>
+<br>
/**<br>
* \brief Assemble and return the camera sensor info<br>
* \param[out] info The camera sensor info<br>
-- <br>
2.29.1<br>
<br>
</blockquote></div></div>