<div dir="ltr"><div dir="ltr">Hi Jean-Michel,<div><br></div><div>Thank you for your work.</div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Wed, 16 Mar 2022 at 10:17, Kieran Bingham via libcamera-devel <<a href="mailto:libcamera-devel@lists.libcamera.org">libcamera-devel@lists.libcamera.org</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 Jean-Michel Hautbois via libcamera-devel (2022-03-16 09:39:51)<br>
> Now that the ancillary links are configured, we can use the CameraLens<br>
> class and control the VCM through the IPA.<br>
> For now, force a default value for the lens position, until the AF<br>
> algorithm is introduced.<br>
> <br>
> Signed-off-by: Jean-Michel Hautbois <<a href="mailto:jeanmichel.hautbois@ideasonboard.com" target="_blank">jeanmichel.hautbois@ideasonboard.com</a>><br>
> ---<br>
> src/ipa/raspberrypi/raspberrypi.cpp | 38 +++++++++++++++++++++++++++--<br>
> 1 file changed, 36 insertions(+), 2 deletions(-)<br>
> <br>
> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp<br>
> index fd8fecb0..0a0b5a3a 100644<br>
> --- a/src/ipa/raspberrypi/raspberrypi.cpp<br>
> +++ b/src/ipa/raspberrypi/raspberrypi.cpp<br>
> @@ -108,6 +108,7 @@ private:<br>
> void setMode(const IPACameraSensorInfo &sensorInfo);<br>
> bool validateSensorControls();<br>
> bool validateIspControls();<br>
> + bool validateLensControls();<br>
> void queueRequest(const ControlList &controls);<br>
> void returnEmbeddedBuffer(unsigned int bufferId);<br>
> void prepareISP(const ipa::RPi::ISPConfig &data);<br>
> @@ -132,6 +133,7 @@ private:<br>
> <br>
> ControlInfoMap sensorCtrls_;<br>
> ControlInfoMap ispCtrls_;<br>
> + ControlInfoMap lensCtrls_;<br>
> ControlList libcameraMetadata_;<br>
> <br>
> /* Camera sensor params. */<br>
> @@ -342,13 +344,14 @@ int IPARPi::configure(const IPACameraSensorInfo &sensorInfo,<br>
> const ipa::RPi::IPAConfig &ipaConfig,<br>
> ControlList *controls)<br>
> {<br>
> - if (entityControls.size() != 2) {<br>
> - LOG(IPARPI, Error) << "No ISP or sensor controls found.";<br>
> + if (entityControls.size() != 3) {<br>
<br>
Ok - so that explains the '2' in the previous patch.<br>
<br>
Feels a bit arbitrary though ... I wonder if it needs to be a map to<br>
make it more explicit to which control list is which...<br></blockquote><div><br></div><div>The entityControls() are vestiges from the days before the mojom IPA interface, and this was the</div><div>only mechanism to pass controls across. I'm all for changing this to a map or something more sensible</div><div>now that we have the flexibility.</div><div><br></div><div>Apart from that, my only real comment for this patch would be, similar to what Kieran has pointed out,</div><div>we need to guard against instances where there is no lens driver.</div><div><br></div><div>Regards,</div><div>Naush</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>
> + LOG(IPARPI, Error) << "No ISP, lens or sensor controls found.";<br>
<br>
Again, what happens on a device with no lens ...<br>
<br>
<br>
> return -1;<br>
> }<br>
> <br>
> sensorCtrls_ = entityControls.at(0);<br>
> ispCtrls_ = entityControls.at(1);<br>
> + lensCtrls_ = entityControls.at(2);<br>
> <br>
> if (!validateSensorControls()) {<br>
> LOG(IPARPI, Error) << "Sensor control validation failed.";<br>
> @@ -360,6 +363,11 @@ int IPARPi::configure(const IPACameraSensorInfo &sensorInfo,<br>
> return -1;<br>
> }<br>
> <br>
> + if (!validateLensControls()) {<br>
<br>
What happens here if there is no lens ...<br>
<br>
<br>
> + LOG(IPARPI, Error) << "Lens control validation failed.";<br>
> + return -1;<br>
> + }<br>
> +<br>
> maxSensorGainCode_ = sensorCtrls_.at(V4L2_CID_ANALOGUE_GAIN).max().get<int32_t>();<br>
> <br>
> /* Setup a metadata ControlList to output metadata. */<br>
> @@ -578,6 +586,23 @@ bool IPARPi::validateIspControls()<br>
> return true;<br>
> }<br>
> <br>
> +bool IPARPi::validateLensControls()<br>
> +{<br>
> + static const uint32_t ctrls[] = {<br>
> + V4L2_CID_FOCUS_ABSOLUTE,<br>
> + };<br>
> +<br>
> + for (auto c : ctrls) {<br>
> + if (lensCtrls_.find(c) == lensCtrls_.end()) {<br>
> + LOG(IPARPI, Error) << "Unable to find lens control "<br>
> + << utils::hex(c);<br>
> + return false;<br>
> + }<br>
> + }<br>
> +<br>
> + return true;<br>
> +}<br>
> +<br>
> /*<br>
> * Converting between enums (used in the libcamera API) and the names that<br>
> * we use to identify different modes. Unfortunately, the conversion tables<br>
> @@ -1066,6 +1091,15 @@ void IPARPi::processStats(unsigned int bufferId)<br>
> <br>
> setDelayedControls.emit(ctrls);<br>
> }<br>
> +<br>
> + /*<br>
> + * Set the focus position<br>
> + * \todo Use an AF algorithm and replace the arbitrary value<br>
> + */<br>
> + ControlList lensCtrls(lensCtrls_);<br>
> + lensCtrls.set(V4L2_CID_FOCUS_ABSOLUTE, static_cast<int32_t>(123));<br>
> + setLensControls.emit(lensCtrls);<br>
> +<br>
<br>
no need for extra blank line here.<br>
<br>
But I guess this is just a 'sample' for the moment anyway.<br>
<br>
It really makes me think changing the IPA API to use a libcamera control<br>
list would be better too - so that the plumbing between is cleaner. But<br>
that's a whole chunk of rework...<br>
<br>
<br>
> }<br>
> <br>
> void IPARPi::applyAWB(const struct AwbStatus *awbStatus, ControlList &ctrls)<br>
> -- <br>
> 2.32.0<br>
><br>
</blockquote></div></div>