<div dir="ltr"><div dir="ltr">Hi Laurent,</div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Mon, 23 Jan 2023 at 09:18, 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>
On Mon, Jan 23, 2023 at 09:02:59AM +0000, Naushir Patuck wrote:<br>
> On Sun, 22 Jan 2023 at 18:54, Laurent Pinchart wrote:<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>
> <br>
> 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>
<br>
Maybe the sensor and ISP controls validation should also move to init()<br>
time then ? :-) This can be done on top of this series, it's not a<br>
blocker.<br></blockquote><div><br></div><div>ISP controls can (and possibly should) move to init, but sensor controls need<br>to remain in configure(). Things like vblank/hblank/exposure/gain limits can<br>and will change across modes.<br><br>I'll make the change as part of this series, it's not going to be too disruptive.<br></div><div><br></div><div>Regards,</div><div>Naush</div><div><br></div><div> <br></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>