<div dir="ltr"><div dir="ltr">Hi Laurent,<div><br></div><div>Thank you for your feedback!</div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Sun, 22 Jan 2023 at 18:54, Laurent Pinchart <<a href="mailto:laurent.pinchart@ideasonboard.com">laurent.pinchart@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">Hi Naush,<br>
<br>
Thank you for the patch.<br>
<br>
On Thu, Jan 19, 2023 at 10:45:34AM +0000, Naushir Patuck via libcamera-devel wrote:<br>
> Pass the available lens controls to the IPA through the configure() function.<br>
> Validate that the V4L2_CID_FOCUS_ABSOLUTE does exist. If it doesn't, log a<br>
> warning message, and do not advertise focus related controls from the IPA.<br>
<br>
Shouldn't this be done at init() time instead of configure() time ? I<br>
don't suppose the presense of a lens controller could change between<br>
different configurations.<br></blockquote><div><br></div><div>Yes, this would be possible to do at init() time. I put it in configure() to<br>be consistent with the sensor and ISP control validation. I doubt lens<br>controls would change between configure() calls, so happy to move it to init()<br>if folks prefer that.<br></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>
> Signed-off-by: Naushir Patuck <<a href="mailto:naush@raspberrypi.com" target="_blank">naush@raspberrypi.com</a>><br>
> Reviewed-by: Nick Hollinghurst <<a href="mailto:nick.hollinghurst@raspberrypi.com" target="_blank">nick.hollinghurst@raspberrypi.com</a>><br>
> Reviewed-by: David Plowman <<a href="mailto:david.plowman@raspberrypi.com" target="_blank">david.plowman@raspberrypi.com</a>><br>
> ---<br>
> include/libcamera/ipa/raspberrypi.mojom | 1 +<br>
> src/ipa/raspberrypi/raspberrypi.cpp | 21 +++++++++++++++++++<br>
> .../pipeline/raspberrypi/raspberrypi.cpp | 2 ++<br>
> 3 files changed, 24 insertions(+)<br>
> <br>
> diff --git a/include/libcamera/ipa/raspberrypi.mojom b/include/libcamera/ipa/raspberrypi.mojom<br>
> index 2a4821fbc0ef..bfacd1275bfb 100644<br>
> --- a/include/libcamera/ipa/raspberrypi.mojom<br>
> +++ b/include/libcamera/ipa/raspberrypi.mojom<br>
> @@ -38,6 +38,7 @@ struct IPAConfig {<br>
> libcamera.SharedFD lsTableHandle;<br>
> libcamera.ControlInfoMap sensorControls;<br>
> libcamera.ControlInfoMap ispControls;<br>
> + libcamera.ControlInfoMap lensControls;<br>
> };<br>
> <br>
> struct IPAConfigResult {<br>
> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp<br>
> index aa18ed750370..bbf3c7dc4a69 100644<br>
> --- a/src/ipa/raspberrypi/raspberrypi.cpp<br>
> +++ b/src/ipa/raspberrypi/raspberrypi.cpp<br>
> @@ -131,6 +131,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 ISPConfig &data);<br>
> @@ -155,6 +156,7 @@ private:<br>
> <br>
> ControlInfoMap sensorCtrls_;<br>
> ControlInfoMap ispCtrls_;<br>
> + ControlInfoMap lensCtrls_;<br>
> bool lensPresent_;<br>
> ControlList libcameraMetadata_;<br>
> <br>
> @@ -394,6 +396,15 @@ int IPARPi::configure(const IPACameraSensorInfo &sensorInfo, const IPAConfig &ip<br>
> return -1;<br>
> }<br>
> <br>
> + if (lensPresent_) {<br>
> + lensCtrls_ = ipaConfig.lensControls;<br>
> + if (!validateLensControls()) {<br>
> + LOG(IPARPI, Warning) << "Lens validation failed, "<br>
> + << "no lens control will be available.";<br>
> + lensPresent_ = false;<br>
> + }<br>
> + }<br>
> +<br>
> maxSensorGainCode_ = sensorCtrls_.at(V4L2_CID_ANALOGUE_GAIN).max().get<int32_t>();<br>
> <br>
> /* Setup a metadata ControlList to output metadata. */<br>
> @@ -648,6 +659,16 @@ bool IPARPi::validateIspControls()<br>
> return true;<br>
> }<br>
> <br>
> +bool IPARPi::validateLensControls()<br>
> +{<br>
> + if (lensCtrls_.find(V4L2_CID_FOCUS_ABSOLUTE) == lensCtrls_.end()) {<br>
> + LOG(IPARPI, Error) << "Unable to find Lens control V4L2_CID_FOCUS_ABSOLUTE";<br>
> + return false;<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>
> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp<br>
> index 9dd36cbaea78..249dedcd8c09 100644<br>
> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp<br>
> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp<br>
> @@ -1617,6 +1617,8 @@ int RPiCameraData::configureIPA(const CameraConfiguration *config, ipa::RPi::IPA<br>
> <br>
> ipaConfig.sensorControls = sensor_->controls();<br>
> ipaConfig.ispControls = isp_[Isp::Input].dev()->controls();<br>
> + if (sensor_->focusLens())<br>
> + ipaConfig.lensControls = sensor_->focusLens()->controls();<br>
> <br>
> /* Always send the user transform to the IPA. */<br>
> ipaConfig.transform = static_cast<unsigned int>(config->transform);<br>
<br>
-- <br>
Regards,<br>
<br>
Laurent Pinchart<br>
</blockquote></div></div>