| Message ID | 20230119104544.9456-5-naush@raspberrypi.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Naush, Thank you for the patch. On Thu, Jan 19, 2023 at 10:45:34AM +0000, Naushir Patuck via libcamera-devel wrote: > Pass the available lens controls to the IPA through the configure() function. > Validate that the V4L2_CID_FOCUS_ABSOLUTE does exist. If it doesn't, log a > warning message, and do not advertise focus related controls from the IPA. Shouldn't this be done at init() time instead of configure() time ? I don't suppose the presense of a lens controller could change between different configurations. > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > Reviewed-by: Nick Hollinghurst <nick.hollinghurst@raspberrypi.com> > Reviewed-by: David Plowman <david.plowman@raspberrypi.com> > --- > include/libcamera/ipa/raspberrypi.mojom | 1 + > src/ipa/raspberrypi/raspberrypi.cpp | 21 +++++++++++++++++++ > .../pipeline/raspberrypi/raspberrypi.cpp | 2 ++ > 3 files changed, 24 insertions(+) > > diff --git a/include/libcamera/ipa/raspberrypi.mojom b/include/libcamera/ipa/raspberrypi.mojom > index 2a4821fbc0ef..bfacd1275bfb 100644 > --- a/include/libcamera/ipa/raspberrypi.mojom > +++ b/include/libcamera/ipa/raspberrypi.mojom > @@ -38,6 +38,7 @@ struct IPAConfig { > libcamera.SharedFD lsTableHandle; > libcamera.ControlInfoMap sensorControls; > libcamera.ControlInfoMap ispControls; > + libcamera.ControlInfoMap lensControls; > }; > > struct IPAConfigResult { > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp > index aa18ed750370..bbf3c7dc4a69 100644 > --- a/src/ipa/raspberrypi/raspberrypi.cpp > +++ b/src/ipa/raspberrypi/raspberrypi.cpp > @@ -131,6 +131,7 @@ private: > void setMode(const IPACameraSensorInfo &sensorInfo); > bool validateSensorControls(); > bool validateIspControls(); > + bool validateLensControls(); > void queueRequest(const ControlList &controls); > void returnEmbeddedBuffer(unsigned int bufferId); > void prepareISP(const ISPConfig &data); > @@ -155,6 +156,7 @@ private: > > ControlInfoMap sensorCtrls_; > ControlInfoMap ispCtrls_; > + ControlInfoMap lensCtrls_; > bool lensPresent_; > ControlList libcameraMetadata_; > > @@ -394,6 +396,15 @@ int IPARPi::configure(const IPACameraSensorInfo &sensorInfo, const IPAConfig &ip > return -1; > } > > + if (lensPresent_) { > + lensCtrls_ = ipaConfig.lensControls; > + if (!validateLensControls()) { > + LOG(IPARPI, Warning) << "Lens validation failed, " > + << "no lens control will be available."; > + lensPresent_ = false; > + } > + } > + > maxSensorGainCode_ = sensorCtrls_.at(V4L2_CID_ANALOGUE_GAIN).max().get<int32_t>(); > > /* Setup a metadata ControlList to output metadata. */ > @@ -648,6 +659,16 @@ bool IPARPi::validateIspControls() > return true; > } > > +bool IPARPi::validateLensControls() > +{ > + if (lensCtrls_.find(V4L2_CID_FOCUS_ABSOLUTE) == lensCtrls_.end()) { > + LOG(IPARPI, Error) << "Unable to find Lens control V4L2_CID_FOCUS_ABSOLUTE"; > + return false; > + } > + > + return true; > +} > + > /* > * Converting between enums (used in the libcamera API) and the names that > * we use to identify different modes. Unfortunately, the conversion tables > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > index 9dd36cbaea78..249dedcd8c09 100644 > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > @@ -1617,6 +1617,8 @@ int RPiCameraData::configureIPA(const CameraConfiguration *config, ipa::RPi::IPA > > ipaConfig.sensorControls = sensor_->controls(); > ipaConfig.ispControls = isp_[Isp::Input].dev()->controls(); > + if (sensor_->focusLens()) > + ipaConfig.lensControls = sensor_->focusLens()->controls(); > > /* Always send the user transform to the IPA. */ > ipaConfig.transform = static_cast<unsigned int>(config->transform);
Hi Laurent, Thank you for your feedback! On Sun, 22 Jan 2023 at 18:54, Laurent Pinchart < laurent.pinchart@ideasonboard.com> wrote: > Hi Naush, > > Thank you for the patch. > > On Thu, Jan 19, 2023 at 10:45:34AM +0000, Naushir Patuck via > libcamera-devel wrote: > > Pass the available lens controls to the IPA through the configure() > function. > > Validate that the V4L2_CID_FOCUS_ABSOLUTE does exist. If it doesn't, log > a > > warning message, and do not advertise focus related controls from the > IPA. > > Shouldn't this be done at init() time instead of configure() time ? I > don't suppose the presense of a lens controller could change between > different configurations. > Yes, this would be possible to do at init() time. I put it in configure() to be consistent with the sensor and ISP control validation. I doubt lens controls would change between configure() calls, so happy to move it to init() if folks prefer that. Regards, Naush > > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > > Reviewed-by: Nick Hollinghurst <nick.hollinghurst@raspberrypi.com> > > Reviewed-by: David Plowman <david.plowman@raspberrypi.com> > > --- > > include/libcamera/ipa/raspberrypi.mojom | 1 + > > src/ipa/raspberrypi/raspberrypi.cpp | 21 +++++++++++++++++++ > > .../pipeline/raspberrypi/raspberrypi.cpp | 2 ++ > > 3 files changed, 24 insertions(+) > > > > diff --git a/include/libcamera/ipa/raspberrypi.mojom > b/include/libcamera/ipa/raspberrypi.mojom > > index 2a4821fbc0ef..bfacd1275bfb 100644 > > --- a/include/libcamera/ipa/raspberrypi.mojom > > +++ b/include/libcamera/ipa/raspberrypi.mojom > > @@ -38,6 +38,7 @@ struct IPAConfig { > > libcamera.SharedFD lsTableHandle; > > libcamera.ControlInfoMap sensorControls; > > libcamera.ControlInfoMap ispControls; > > + libcamera.ControlInfoMap lensControls; > > }; > > > > struct IPAConfigResult { > > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp > b/src/ipa/raspberrypi/raspberrypi.cpp > > index aa18ed750370..bbf3c7dc4a69 100644 > > --- a/src/ipa/raspberrypi/raspberrypi.cpp > > +++ b/src/ipa/raspberrypi/raspberrypi.cpp > > @@ -131,6 +131,7 @@ private: > > void setMode(const IPACameraSensorInfo &sensorInfo); > > bool validateSensorControls(); > > bool validateIspControls(); > > + bool validateLensControls(); > > void queueRequest(const ControlList &controls); > > void returnEmbeddedBuffer(unsigned int bufferId); > > void prepareISP(const ISPConfig &data); > > @@ -155,6 +156,7 @@ private: > > > > ControlInfoMap sensorCtrls_; > > ControlInfoMap ispCtrls_; > > + ControlInfoMap lensCtrls_; > > bool lensPresent_; > > ControlList libcameraMetadata_; > > > > @@ -394,6 +396,15 @@ int IPARPi::configure(const IPACameraSensorInfo > &sensorInfo, const IPAConfig &ip > > return -1; > > } > > > > + if (lensPresent_) { > > + lensCtrls_ = ipaConfig.lensControls; > > + if (!validateLensControls()) { > > + LOG(IPARPI, Warning) << "Lens validation failed, " > > + << "no lens control will be > available."; > > + lensPresent_ = false; > > + } > > + } > > + > > maxSensorGainCode_ = > sensorCtrls_.at(V4L2_CID_ANALOGUE_GAIN).max().get<int32_t>(); > > > > /* Setup a metadata ControlList to output metadata. */ > > @@ -648,6 +659,16 @@ bool IPARPi::validateIspControls() > > return true; > > } > > > > +bool IPARPi::validateLensControls() > > +{ > > + if (lensCtrls_.find(V4L2_CID_FOCUS_ABSOLUTE) == lensCtrls_.end()) { > > + LOG(IPARPI, Error) << "Unable to find Lens control > V4L2_CID_FOCUS_ABSOLUTE"; > > + return false; > > + } > > + > > + return true; > > +} > > + > > /* > > * Converting between enums (used in the libcamera API) and the names > that > > * we use to identify different modes. Unfortunately, the conversion > tables > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > index 9dd36cbaea78..249dedcd8c09 100644 > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > @@ -1617,6 +1617,8 @@ int RPiCameraData::configureIPA(const > CameraConfiguration *config, ipa::RPi::IPA > > > > ipaConfig.sensorControls = sensor_->controls(); > > ipaConfig.ispControls = isp_[Isp::Input].dev()->controls(); > > + if (sensor_->focusLens()) > > + ipaConfig.lensControls = sensor_->focusLens()->controls(); > > > > /* Always send the user transform to the IPA. */ > > ipaConfig.transform = static_cast<unsigned int>(config->transform); > > -- > Regards, > > Laurent Pinchart >
Hi Naush, On Mon, Jan 23, 2023 at 09:02:59AM +0000, Naushir Patuck wrote: > On Sun, 22 Jan 2023 at 18:54, Laurent Pinchart wrote: > > On Thu, Jan 19, 2023 at 10:45:34AM +0000, Naushir Patuck via libcamera-devel wrote: > > > Pass the available lens controls to the IPA through the configure() function. > > > Validate that the V4L2_CID_FOCUS_ABSOLUTE does exist. If it doesn't, log a > > > warning message, and do not advertise focus related controls from the IPA. > > > > Shouldn't this be done at init() time instead of configure() time ? I > > don't suppose the presense of a lens controller could change between > > different configurations. > > Yes, this would be possible to do at init() time. I put it in configure() to > be consistent with the sensor and ISP control validation. I doubt lens > controls would change between configure() calls, so happy to move it to init() > if folks prefer that. Maybe the sensor and ISP controls validation should also move to init() time then ? :-) This can be done on top of this series, it's not a blocker. > > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > > > Reviewed-by: Nick Hollinghurst <nick.hollinghurst@raspberrypi.com> > > > Reviewed-by: David Plowman <david.plowman@raspberrypi.com> > > > --- > > > include/libcamera/ipa/raspberrypi.mojom | 1 + > > > src/ipa/raspberrypi/raspberrypi.cpp | 21 +++++++++++++++++++ > > > .../pipeline/raspberrypi/raspberrypi.cpp | 2 ++ > > > 3 files changed, 24 insertions(+) > > > > > > diff --git a/include/libcamera/ipa/raspberrypi.mojom b/include/libcamera/ipa/raspberrypi.mojom > > > index 2a4821fbc0ef..bfacd1275bfb 100644 > > > --- a/include/libcamera/ipa/raspberrypi.mojom > > > +++ b/include/libcamera/ipa/raspberrypi.mojom > > > @@ -38,6 +38,7 @@ struct IPAConfig { > > > libcamera.SharedFD lsTableHandle; > > > libcamera.ControlInfoMap sensorControls; > > > libcamera.ControlInfoMap ispControls; > > > + libcamera.ControlInfoMap lensControls; > > > }; > > > > > > struct IPAConfigResult { > > > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp > > > index aa18ed750370..bbf3c7dc4a69 100644 > > > --- a/src/ipa/raspberrypi/raspberrypi.cpp > > > +++ b/src/ipa/raspberrypi/raspberrypi.cpp > > > @@ -131,6 +131,7 @@ private: > > > void setMode(const IPACameraSensorInfo &sensorInfo); > > > bool validateSensorControls(); > > > bool validateIspControls(); > > > + bool validateLensControls(); > > > void queueRequest(const ControlList &controls); > > > void returnEmbeddedBuffer(unsigned int bufferId); > > > void prepareISP(const ISPConfig &data); > > > @@ -155,6 +156,7 @@ private: > > > > > > ControlInfoMap sensorCtrls_; > > > ControlInfoMap ispCtrls_; > > > + ControlInfoMap lensCtrls_; > > > bool lensPresent_; > > > ControlList libcameraMetadata_; > > > > > > @@ -394,6 +396,15 @@ int IPARPi::configure(const IPACameraSensorInfo &sensorInfo, const IPAConfig &ip > > > return -1; > > > } > > > > > > + if (lensPresent_) { > > > + lensCtrls_ = ipaConfig.lensControls; > > > + if (!validateLensControls()) { > > > + LOG(IPARPI, Warning) << "Lens validation failed, " > > > + << "no lens control will be available."; > > > + lensPresent_ = false; > > > + } > > > + } > > > + > > > maxSensorGainCode_ = sensorCtrls_.at(V4L2_CID_ANALOGUE_GAIN).max().get<int32_t>(); > > > > > > /* Setup a metadata ControlList to output metadata. */ > > > @@ -648,6 +659,16 @@ bool IPARPi::validateIspControls() > > > return true; > > > } > > > > > > +bool IPARPi::validateLensControls() > > > +{ > > > + if (lensCtrls_.find(V4L2_CID_FOCUS_ABSOLUTE) == lensCtrls_.end()) { > > > + LOG(IPARPI, Error) << "Unable to find Lens control V4L2_CID_FOCUS_ABSOLUTE"; > > > + return false; > > > + } > > > + > > > + return true; > > > +} > > > + > > > /* > > > * Converting between enums (used in the libcamera API) and the names that > > > * we use to identify different modes. Unfortunately, the conversion tables > > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > > index 9dd36cbaea78..249dedcd8c09 100644 > > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > > @@ -1617,6 +1617,8 @@ int RPiCameraData::configureIPA(const CameraConfiguration *config, ipa::RPi::IPA > > > > > > ipaConfig.sensorControls = sensor_->controls(); > > > ipaConfig.ispControls = isp_[Isp::Input].dev()->controls(); > > > + if (sensor_->focusLens()) > > > + ipaConfig.lensControls = sensor_->focusLens()->controls(); > > > > > > /* Always send the user transform to the IPA. */ > > > ipaConfig.transform = static_cast<unsigned int>(config->transform);
Hi Laurent, On Mon, 23 Jan 2023 at 09:18, Laurent Pinchart < laurent.pinchart@ideasonboard.com> wrote: > Hi Naush, > > On Mon, Jan 23, 2023 at 09:02:59AM +0000, Naushir Patuck wrote: > > On Sun, 22 Jan 2023 at 18:54, Laurent Pinchart wrote: > > > On Thu, Jan 19, 2023 at 10:45:34AM +0000, Naushir Patuck via > libcamera-devel wrote: > > > > Pass the available lens controls to the IPA through the configure() > function. > > > > Validate that the V4L2_CID_FOCUS_ABSOLUTE does exist. If it doesn't, > log a > > > > warning message, and do not advertise focus related controls from > the IPA. > > > > > > Shouldn't this be done at init() time instead of configure() time ? I > > > don't suppose the presense of a lens controller could change between > > > different configurations. > > > > Yes, this would be possible to do at init() time. I put it in > configure() to > > be consistent with the sensor and ISP control validation. I doubt lens > > controls would change between configure() calls, so happy to move it to > init() > > if folks prefer that. > > Maybe the sensor and ISP controls validation should also move to init() > time then ? :-) This can be done on top of this series, it's not a > blocker. > ISP controls can (and possibly should) move to init, but sensor controls need to remain in configure(). Things like vblank/hblank/exposure/gain limits can and will change across modes. I'll make the change as part of this series, it's not going to be too disruptive. Regards, Naush > > > > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > > > > Reviewed-by: Nick Hollinghurst <nick.hollinghurst@raspberrypi.com> > > > > Reviewed-by: David Plowman <david.plowman@raspberrypi.com> > > > > --- > > > > include/libcamera/ipa/raspberrypi.mojom | 1 + > > > > src/ipa/raspberrypi/raspberrypi.cpp | 21 > +++++++++++++++++++ > > > > .../pipeline/raspberrypi/raspberrypi.cpp | 2 ++ > > > > 3 files changed, 24 insertions(+) > > > > > > > > diff --git a/include/libcamera/ipa/raspberrypi.mojom > b/include/libcamera/ipa/raspberrypi.mojom > > > > index 2a4821fbc0ef..bfacd1275bfb 100644 > > > > --- a/include/libcamera/ipa/raspberrypi.mojom > > > > +++ b/include/libcamera/ipa/raspberrypi.mojom > > > > @@ -38,6 +38,7 @@ struct IPAConfig { > > > > libcamera.SharedFD lsTableHandle; > > > > libcamera.ControlInfoMap sensorControls; > > > > libcamera.ControlInfoMap ispControls; > > > > + libcamera.ControlInfoMap lensControls; > > > > }; > > > > > > > > struct IPAConfigResult { > > > > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp > b/src/ipa/raspberrypi/raspberrypi.cpp > > > > index aa18ed750370..bbf3c7dc4a69 100644 > > > > --- a/src/ipa/raspberrypi/raspberrypi.cpp > > > > +++ b/src/ipa/raspberrypi/raspberrypi.cpp > > > > @@ -131,6 +131,7 @@ private: > > > > void setMode(const IPACameraSensorInfo &sensorInfo); > > > > bool validateSensorControls(); > > > > bool validateIspControls(); > > > > + bool validateLensControls(); > > > > void queueRequest(const ControlList &controls); > > > > void returnEmbeddedBuffer(unsigned int bufferId); > > > > void prepareISP(const ISPConfig &data); > > > > @@ -155,6 +156,7 @@ private: > > > > > > > > ControlInfoMap sensorCtrls_; > > > > ControlInfoMap ispCtrls_; > > > > + ControlInfoMap lensCtrls_; > > > > bool lensPresent_; > > > > ControlList libcameraMetadata_; > > > > > > > > @@ -394,6 +396,15 @@ int IPARPi::configure(const IPACameraSensorInfo > &sensorInfo, const IPAConfig &ip > > > > return -1; > > > > } > > > > > > > > + if (lensPresent_) { > > > > + lensCtrls_ = ipaConfig.lensControls; > > > > + if (!validateLensControls()) { > > > > + LOG(IPARPI, Warning) << "Lens validation > failed, " > > > > + << "no lens control will > be available."; > > > > + lensPresent_ = false; > > > > + } > > > > + } > > > > + > > > > maxSensorGainCode_ = > sensorCtrls_.at(V4L2_CID_ANALOGUE_GAIN).max().get<int32_t>(); > > > > > > > > /* Setup a metadata ControlList to output metadata. */ > > > > @@ -648,6 +659,16 @@ bool IPARPi::validateIspControls() > > > > return true; > > > > } > > > > > > > > +bool IPARPi::validateLensControls() > > > > +{ > > > > + if (lensCtrls_.find(V4L2_CID_FOCUS_ABSOLUTE) == > lensCtrls_.end()) { > > > > + LOG(IPARPI, Error) << "Unable to find Lens control > V4L2_CID_FOCUS_ABSOLUTE"; > > > > + return false; > > > > + } > > > > + > > > > + return true; > > > > +} > > > > + > > > > /* > > > > * Converting between enums (used in the libcamera API) and the > names that > > > > * we use to identify different modes. Unfortunately, the > conversion tables > > > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > > > index 9dd36cbaea78..249dedcd8c09 100644 > > > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > > > @@ -1617,6 +1617,8 @@ int RPiCameraData::configureIPA(const > CameraConfiguration *config, ipa::RPi::IPA > > > > > > > > ipaConfig.sensorControls = sensor_->controls(); > > > > ipaConfig.ispControls = isp_[Isp::Input].dev()->controls(); > > > > + if (sensor_->focusLens()) > > > > + ipaConfig.lensControls = > sensor_->focusLens()->controls(); > > > > > > > > /* Always send the user transform to the IPA. */ > > > > ipaConfig.transform = static_cast<unsigned > int>(config->transform); > > -- > Regards, > > Laurent Pinchart >
On Mon, 23 Jan 2023 at 09:22, Naushir Patuck <naush@raspberrypi.com> wrote: > Hi Laurent, > > On Mon, 23 Jan 2023 at 09:18, Laurent Pinchart < > laurent.pinchart@ideasonboard.com> wrote: > >> Hi Naush, >> >> On Mon, Jan 23, 2023 at 09:02:59AM +0000, Naushir Patuck wrote: >> > On Sun, 22 Jan 2023 at 18:54, Laurent Pinchart wrote: >> > > On Thu, Jan 19, 2023 at 10:45:34AM +0000, Naushir Patuck via >> libcamera-devel wrote: >> > > > Pass the available lens controls to the IPA through the configure() >> function. >> > > > Validate that the V4L2_CID_FOCUS_ABSOLUTE does exist. If it >> doesn't, log a >> > > > warning message, and do not advertise focus related controls from >> the IPA. >> > > >> > > Shouldn't this be done at init() time instead of configure() time ? I >> > > don't suppose the presense of a lens controller could change between >> > > different configurations. >> > >> > Yes, this would be possible to do at init() time. I put it in >> configure() to >> > be consistent with the sensor and ISP control validation. I doubt lens >> > controls would change between configure() calls, so happy to move it to >> init() >> > if folks prefer that. >> >> Maybe the sensor and ISP controls validation should also move to init() >> time then ? :-) This can be done on top of this series, it's not a >> blocker. >> > > ISP controls can (and possibly should) move to init, but sensor controls > need > to remain in configure(). Things like vblank/hblank/exposure/gain limits > can > and will change across modes. > > I'll make the change as part of this series, it's not going to be too > disruptive. > Can I backtrack on my statement please? :-) There is a bit more restructuring involved to make this change - specifically the devices are not yet opened at the point of ipa init(), so there are no controls to be passed in. Nothing too troublesome, but I think it makes sense to do this as a separate piece of work on top. Naush > > Regards, > Naush > > > >> >> > > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> >> > > > Reviewed-by: Nick Hollinghurst <nick.hollinghurst@raspberrypi.com> >> > > > Reviewed-by: David Plowman <david.plowman@raspberrypi.com> >> > > > --- >> > > > include/libcamera/ipa/raspberrypi.mojom | 1 + >> > > > src/ipa/raspberrypi/raspberrypi.cpp | 21 >> +++++++++++++++++++ >> > > > .../pipeline/raspberrypi/raspberrypi.cpp | 2 ++ >> > > > 3 files changed, 24 insertions(+) >> > > > >> > > > diff --git a/include/libcamera/ipa/raspberrypi.mojom >> b/include/libcamera/ipa/raspberrypi.mojom >> > > > index 2a4821fbc0ef..bfacd1275bfb 100644 >> > > > --- a/include/libcamera/ipa/raspberrypi.mojom >> > > > +++ b/include/libcamera/ipa/raspberrypi.mojom >> > > > @@ -38,6 +38,7 @@ struct IPAConfig { >> > > > libcamera.SharedFD lsTableHandle; >> > > > libcamera.ControlInfoMap sensorControls; >> > > > libcamera.ControlInfoMap ispControls; >> > > > + libcamera.ControlInfoMap lensControls; >> > > > }; >> > > > >> > > > struct IPAConfigResult { >> > > > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp >> b/src/ipa/raspberrypi/raspberrypi.cpp >> > > > index aa18ed750370..bbf3c7dc4a69 100644 >> > > > --- a/src/ipa/raspberrypi/raspberrypi.cpp >> > > > +++ b/src/ipa/raspberrypi/raspberrypi.cpp >> > > > @@ -131,6 +131,7 @@ private: >> > > > void setMode(const IPACameraSensorInfo &sensorInfo); >> > > > bool validateSensorControls(); >> > > > bool validateIspControls(); >> > > > + bool validateLensControls(); >> > > > void queueRequest(const ControlList &controls); >> > > > void returnEmbeddedBuffer(unsigned int bufferId); >> > > > void prepareISP(const ISPConfig &data); >> > > > @@ -155,6 +156,7 @@ private: >> > > > >> > > > ControlInfoMap sensorCtrls_; >> > > > ControlInfoMap ispCtrls_; >> > > > + ControlInfoMap lensCtrls_; >> > > > bool lensPresent_; >> > > > ControlList libcameraMetadata_; >> > > > >> > > > @@ -394,6 +396,15 @@ int IPARPi::configure(const >> IPACameraSensorInfo &sensorInfo, const IPAConfig &ip >> > > > return -1; >> > > > } >> > > > >> > > > + if (lensPresent_) { >> > > > + lensCtrls_ = ipaConfig.lensControls; >> > > > + if (!validateLensControls()) { >> > > > + LOG(IPARPI, Warning) << "Lens validation >> failed, " >> > > > + << "no lens control will >> be available."; >> > > > + lensPresent_ = false; >> > > > + } >> > > > + } >> > > > + >> > > > maxSensorGainCode_ = >> sensorCtrls_.at(V4L2_CID_ANALOGUE_GAIN).max().get<int32_t>(); >> > > > >> > > > /* Setup a metadata ControlList to output metadata. */ >> > > > @@ -648,6 +659,16 @@ bool IPARPi::validateIspControls() >> > > > return true; >> > > > } >> > > > >> > > > +bool IPARPi::validateLensControls() >> > > > +{ >> > > > + if (lensCtrls_.find(V4L2_CID_FOCUS_ABSOLUTE) == >> lensCtrls_.end()) { >> > > > + LOG(IPARPI, Error) << "Unable to find Lens control >> V4L2_CID_FOCUS_ABSOLUTE"; >> > > > + return false; >> > > > + } >> > > > + >> > > > + return true; >> > > > +} >> > > > + >> > > > /* >> > > > * Converting between enums (used in the libcamera API) and the >> names that >> > > > * we use to identify different modes. Unfortunately, the >> conversion tables >> > > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp >> b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp >> > > > index 9dd36cbaea78..249dedcd8c09 100644 >> > > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp >> > > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp >> > > > @@ -1617,6 +1617,8 @@ int RPiCameraData::configureIPA(const >> CameraConfiguration *config, ipa::RPi::IPA >> > > > >> > > > ipaConfig.sensorControls = sensor_->controls(); >> > > > ipaConfig.ispControls = isp_[Isp::Input].dev()->controls(); >> > > > + if (sensor_->focusLens()) >> > > > + ipaConfig.lensControls = >> sensor_->focusLens()->controls(); >> > > > >> > > > /* Always send the user transform to the IPA. */ >> > > > ipaConfig.transform = static_cast<unsigned >> int>(config->transform); >> >> -- >> Regards, >> >> Laurent Pinchart >> >
Hi Naush, On Mon, Jan 23, 2023 at 10:39:16AM +0000, Naushir Patuck wrote: > On Mon, 23 Jan 2023 at 09:22, Naushir Patuck wrote: > > On Mon, 23 Jan 2023 at 09:18, Laurent Pinchart wrote: > >> On Mon, Jan 23, 2023 at 09:02:59AM +0000, Naushir Patuck wrote: > >> > On Sun, 22 Jan 2023 at 18:54, Laurent Pinchart wrote: > >> > > On Thu, Jan 19, 2023 at 10:45:34AM +0000, Naushir Patuck via libcamera-devel wrote: > >> > > > Pass the available lens controls to the IPA through the configure() function. > >> > > > Validate that the V4L2_CID_FOCUS_ABSOLUTE does exist. If it doesn't, log a > >> > > > warning message, and do not advertise focus related controls from the IPA. > >> > > > >> > > Shouldn't this be done at init() time instead of configure() time ? I > >> > > don't suppose the presense of a lens controller could change between > >> > > different configurations. > >> > > >> > Yes, this would be possible to do at init() time. I put it in configure() to > >> > be consistent with the sensor and ISP control validation. I doubt lens > >> > controls would change between configure() calls, so happy to move it to init() > >> > if folks prefer that. > >> > >> Maybe the sensor and ISP controls validation should also move to init() > >> time then ? :-) This can be done on top of this series, it's not a > >> blocker. > > > > ISP controls can (and possibly should) move to init, but sensor controls need > > to remain in configure(). Things like vblank/hblank/exposure/gain limits can > > and will change across modes. > > > > I'll make the change as part of this series, it's not going to be too > > disruptive. > > Can I backtrack on my statement please? :-) > > There is a bit more restructuring involved to make this change - specifically > the devices are not yet opened at the point of ipa init(), so there are no > controls to be passed in. Nothing too troublesome, but I think it makes sense > to do this as a separate piece of work on top. Sure, no problem, this can be done on top. You can just add a todo comment in the next version of this patch. > >> > > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > >> > > > Reviewed-by: Nick Hollinghurst <nick.hollinghurst@raspberrypi.com> > >> > > > Reviewed-by: David Plowman <david.plowman@raspberrypi.com> > >> > > > --- > >> > > > include/libcamera/ipa/raspberrypi.mojom | 1 + > >> > > > src/ipa/raspberrypi/raspberrypi.cpp | 21 +++++++++++++++++++ > >> > > > .../pipeline/raspberrypi/raspberrypi.cpp | 2 ++ > >> > > > 3 files changed, 24 insertions(+) > >> > > > > >> > > > diff --git a/include/libcamera/ipa/raspberrypi.mojom b/include/libcamera/ipa/raspberrypi.mojom > >> > > > index 2a4821fbc0ef..bfacd1275bfb 100644 > >> > > > --- a/include/libcamera/ipa/raspberrypi.mojom > >> > > > +++ b/include/libcamera/ipa/raspberrypi.mojom > >> > > > @@ -38,6 +38,7 @@ struct IPAConfig { > >> > > > libcamera.SharedFD lsTableHandle; > >> > > > libcamera.ControlInfoMap sensorControls; > >> > > > libcamera.ControlInfoMap ispControls; > >> > > > + libcamera.ControlInfoMap lensControls; > >> > > > }; > >> > > > > >> > > > struct IPAConfigResult { > >> > > > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp > >> > > > index aa18ed750370..bbf3c7dc4a69 100644 > >> > > > --- a/src/ipa/raspberrypi/raspberrypi.cpp > >> > > > +++ b/src/ipa/raspberrypi/raspberrypi.cpp > >> > > > @@ -131,6 +131,7 @@ private: > >> > > > void setMode(const IPACameraSensorInfo &sensorInfo); > >> > > > bool validateSensorControls(); > >> > > > bool validateIspControls(); > >> > > > + bool validateLensControls(); > >> > > > void queueRequest(const ControlList &controls); > >> > > > void returnEmbeddedBuffer(unsigned int bufferId); > >> > > > void prepareISP(const ISPConfig &data); > >> > > > @@ -155,6 +156,7 @@ private: > >> > > > > >> > > > ControlInfoMap sensorCtrls_; > >> > > > ControlInfoMap ispCtrls_; > >> > > > + ControlInfoMap lensCtrls_; > >> > > > bool lensPresent_; > >> > > > ControlList libcameraMetadata_; > >> > > > > >> > > > @@ -394,6 +396,15 @@ int IPARPi::configure(const IPACameraSensorInfo &sensorInfo, const IPAConfig &ip > >> > > > return -1; > >> > > > } > >> > > > > >> > > > + if (lensPresent_) { > >> > > > + lensCtrls_ = ipaConfig.lensControls; > >> > > > + if (!validateLensControls()) { > >> > > > + LOG(IPARPI, Warning) << "Lens validation failed, " > >> > > > + << "no lens control will be available."; > >> > > > + lensPresent_ = false; > >> > > > + } > >> > > > + } > >> > > > + > >> > > > maxSensorGainCode_ = sensorCtrls_.at(V4L2_CID_ANALOGUE_GAIN).max().get<int32_t>(); > >> > > > > >> > > > /* Setup a metadata ControlList to output metadata. */ > >> > > > @@ -648,6 +659,16 @@ bool IPARPi::validateIspControls() > >> > > > return true; > >> > > > } > >> > > > > >> > > > +bool IPARPi::validateLensControls() > >> > > > +{ > >> > > > + if (lensCtrls_.find(V4L2_CID_FOCUS_ABSOLUTE) == lensCtrls_.end()) { > >> > > > + LOG(IPARPI, Error) << "Unable to find Lens control V4L2_CID_FOCUS_ABSOLUTE"; > >> > > > + return false; > >> > > > + } > >> > > > + > >> > > > + return true; > >> > > > +} > >> > > > + > >> > > > /* > >> > > > * Converting between enums (used in the libcamera API) and the names that > >> > > > * we use to identify different modes. Unfortunately, the conversion tables > >> > > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > >> > > > index 9dd36cbaea78..249dedcd8c09 100644 > >> > > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > >> > > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > >> > > > @@ -1617,6 +1617,8 @@ int RPiCameraData::configureIPA(const CameraConfiguration *config, ipa::RPi::IPA > >> > > > > >> > > > ipaConfig.sensorControls = sensor_->controls(); > >> > > > ipaConfig.ispControls = isp_[Isp::Input].dev()->controls(); > >> > > > + if (sensor_->focusLens()) > >> > > > + ipaConfig.lensControls = sensor_->focusLens()->controls(); > >> > > > > >> > > > /* Always send the user transform to the IPA. */ > >> > > > ipaConfig.transform = static_cast<unsigned int>(config->transform);
diff --git a/include/libcamera/ipa/raspberrypi.mojom b/include/libcamera/ipa/raspberrypi.mojom index 2a4821fbc0ef..bfacd1275bfb 100644 --- a/include/libcamera/ipa/raspberrypi.mojom +++ b/include/libcamera/ipa/raspberrypi.mojom @@ -38,6 +38,7 @@ struct IPAConfig { libcamera.SharedFD lsTableHandle; libcamera.ControlInfoMap sensorControls; libcamera.ControlInfoMap ispControls; + libcamera.ControlInfoMap lensControls; }; struct IPAConfigResult { diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp index aa18ed750370..bbf3c7dc4a69 100644 --- a/src/ipa/raspberrypi/raspberrypi.cpp +++ b/src/ipa/raspberrypi/raspberrypi.cpp @@ -131,6 +131,7 @@ private: void setMode(const IPACameraSensorInfo &sensorInfo); bool validateSensorControls(); bool validateIspControls(); + bool validateLensControls(); void queueRequest(const ControlList &controls); void returnEmbeddedBuffer(unsigned int bufferId); void prepareISP(const ISPConfig &data); @@ -155,6 +156,7 @@ private: ControlInfoMap sensorCtrls_; ControlInfoMap ispCtrls_; + ControlInfoMap lensCtrls_; bool lensPresent_; ControlList libcameraMetadata_; @@ -394,6 +396,15 @@ int IPARPi::configure(const IPACameraSensorInfo &sensorInfo, const IPAConfig &ip return -1; } + if (lensPresent_) { + lensCtrls_ = ipaConfig.lensControls; + if (!validateLensControls()) { + LOG(IPARPI, Warning) << "Lens validation failed, " + << "no lens control will be available."; + lensPresent_ = false; + } + } + maxSensorGainCode_ = sensorCtrls_.at(V4L2_CID_ANALOGUE_GAIN).max().get<int32_t>(); /* Setup a metadata ControlList to output metadata. */ @@ -648,6 +659,16 @@ bool IPARPi::validateIspControls() return true; } +bool IPARPi::validateLensControls() +{ + if (lensCtrls_.find(V4L2_CID_FOCUS_ABSOLUTE) == lensCtrls_.end()) { + LOG(IPARPI, Error) << "Unable to find Lens control V4L2_CID_FOCUS_ABSOLUTE"; + return false; + } + + return true; +} + /* * Converting between enums (used in the libcamera API) and the names that * we use to identify different modes. Unfortunately, the conversion tables diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp index 9dd36cbaea78..249dedcd8c09 100644 --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp @@ -1617,6 +1617,8 @@ int RPiCameraData::configureIPA(const CameraConfiguration *config, ipa::RPi::IPA ipaConfig.sensorControls = sensor_->controls(); ipaConfig.ispControls = isp_[Isp::Input].dev()->controls(); + if (sensor_->focusLens()) + ipaConfig.lensControls = sensor_->focusLens()->controls(); /* Always send the user transform to the IPA. */ ipaConfig.transform = static_cast<unsigned int>(config->transform);