<div dir="ltr"><div dir="ltr">Hi Kieran,</div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Wed, 13 Apr 2022 at 10:38, Kieran Bingham <<a href="mailto:kieran.bingham@ideasonboard.com">kieran.bingham@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 David, Naush,<br>
<br>
Quoting David Plowman (2021-09-22 14:29:15)<br>
> These changes retrieve the correct value for sensitivity of the mode<br>
> selected for the sensor. This value is known to the CamHelper which<br>
> passes it across to the pipeline handler so that it can be set<br>
> correctly in the CameraConfiguration.<br>
> <br>
> Signed-off-by: David Plowman <<a href="mailto:david.plowman@raspberrypi.com" target="_blank">david.plowman@raspberrypi.com</a>><br>
> ---<br>
> include/libcamera/ipa/raspberrypi.mojom | 7 ++++++-<br>
> src/ipa/raspberrypi/raspberrypi.cpp | 7 +++++--<br>
> src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 10 +++++++---<br>
> 3 files changed, 18 insertions(+), 6 deletions(-)<br>
> <br>
> diff --git a/include/libcamera/ipa/raspberrypi.mojom b/include/libcamera/ipa/raspberrypi.mojom<br>
> index e453d46c..a92a76f8 100644<br>
> --- a/include/libcamera/ipa/raspberrypi.mojom<br>
> +++ b/include/libcamera/ipa/raspberrypi.mojom<br>
> @@ -38,6 +38,10 @@ struct IPAConfig {<br>
> libcamera.FileDescriptor lsTableHandle;<br>
> };<br>
> <br>
> +struct IPAConfigResult {<br>
> + float modeSensitivity;<br>
> +};<br>
> +<br>
> struct StartConfig {<br>
> libcamera.ControlList controls;<br>
> int32 dropFrameCount;<br>
> @@ -57,6 +61,7 @@ interface IPARPiInterface {<br>
> * \param[in] entityControls Controls provided by the pipeline entities<br>
> * \param[in] ipaConfig Pipeline-handler-specific configuration data<br>
> * \param[out] controls Controls to apply by the pipeline entity<br>
> + * \param[out] result Other results that the pipeline handler may require<br>
> *<br>
> * This function shall be called when the camera is configured to inform<br>
> * the IPA of the camera's streams and the sensor settings.<br>
> @@ -71,7 +76,7 @@ interface IPARPiInterface {<br>
> map<uint32, libcamera.IPAStream> streamConfig,<br>
> map<uint32, libcamera.ControlInfoMap> entityControls,<br>
> IPAConfig ipaConfig)<br>
> - => (int32 ret, libcamera.ControlList controls);<br>
> + => (int32 ret, libcamera.ControlList controls, IPAConfigResult result);<br>
> <br>
> /**<br>
> * \fn mapBuffers()<br>
> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp<br>
> index 047123ab..796e6d15 100644<br>
> --- a/src/ipa/raspberrypi/raspberrypi.cpp<br>
> +++ b/src/ipa/raspberrypi/raspberrypi.cpp<br>
> @@ -97,7 +97,7 @@ public:<br>
> const std::map<unsigned int, IPAStream> &streamConfig,<br>
> const std::map<unsigned int, ControlInfoMap> &entityControls,<br>
> const ipa::RPi::IPAConfig &data,<br>
> - ControlList *controls) override;<br>
> + ControlList *controls, ipa::RPi::IPAConfigResult *result) override;<br>
> void mapBuffers(const std::vector<IPABuffer> &buffers) override;<br>
> void unmapBuffers(const std::vector<unsigned int> &ids) override;<br>
> void signalStatReady(const uint32_t bufferId) override;<br>
> @@ -337,7 +337,7 @@ int IPARPi::configure(const IPACameraSensorInfo &sensorInfo,<br>
> [[maybe_unused]] const std::map<unsigned int, IPAStream> &streamConfig,<br>
> const std::map<unsigned int, ControlInfoMap> &entityControls,<br>
> const ipa::RPi::IPAConfig &ipaConfig,<br>
> - ControlList *controls)<br>
> + ControlList *controls, ipa::RPi::IPAConfigResult *result)<br>
> {<br>
> if (entityControls.size() != 2) {<br>
> LOG(IPARPI, Error) << "No ISP or sensor controls found.";<br>
> @@ -389,6 +389,9 @@ int IPARPi::configure(const IPACameraSensorInfo &sensorInfo,<br>
> /* Pass the camera mode to the CamHelper to setup algorithms. */<br>
> helper_->SetCameraMode(mode_);<br>
> <br>
> + /* The pipeline handler passes out the mode's sensitivity. */<br>
> + result->modeSensitivity = mode_.sensitivity;<br>
> +<br>
> if (firstStart_) {<br>
> /* Supply initial values for frame durations. */<br>
> applyFrameDurations(defaultMinFrameDuration, defaultMaxFrameDuration);<br>
> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp<br>
> index 0bdfa727..caf0030e 100644<br>
> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp<br>
> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp<br>
> @@ -147,7 +147,7 @@ public:<br>
> void frameStarted(uint32_t sequence);<br>
> <br>
> int loadIPA(ipa::RPi::SensorConfig *sensorConfig);<br>
> - int configureIPA(const CameraConfiguration *config);<br>
> + int configureIPA(CameraConfiguration *config);<br>
> <br>
> void statsMetadataComplete(uint32_t bufferId, const ControlList &controls);<br>
> void runIsp(uint32_t bufferId);<br>
> @@ -1250,7 +1250,7 @@ int RPiCameraData::loadIPA(ipa::RPi::SensorConfig *sensorConfig)<br>
> return ipa_->init(settings, sensorConfig);<br>
> }<br>
> <br>
> -int RPiCameraData::configureIPA(const CameraConfiguration *config)<br>
> +int RPiCameraData::configureIPA(CameraConfiguration *config)<br>
> {<br>
> /* We know config must be an RPiCameraConfiguration. */<br>
> const RPiCameraConfiguration *rpiConfig =<br>
> @@ -1299,13 +1299,17 @@ int RPiCameraData::configureIPA(const CameraConfiguration *config)<br>
> <br>
> /* Ready the IPA - it must know about the sensor resolution. */<br>
> ControlList controls;<br>
> + ipa::RPi::IPAConfigResult result;<br>
> ret = ipa_->configure(sensorInfo_, streamConfig, entityControls, ipaConfig,<br>
> - &controls);<br>
> + &controls, &result);<br>
<br>
I've just rebased this series to master to facilitate merging, and with<br>
fresh eyes I can't help but wonder if this value shouldn't be returned<br>
in the validate() phase. (Not sure if this has been asked before / yet).<br>
<br>
Is there anything that prevents us adding a validate() to the IPA<br>
interface to allow validating the configuration and at that point,<br>
setting the mode sensitivity? Or can this value /only/ be determined<br>
when configuring?<br></blockquote><div><br></div><div>I can't think of any reason that this couldn't be done in validate(), but David</div><div>might have some reasons. However, is an application required to call</div><div>CameraConfiguration::validate() directly? I can see that it does get called<br></div><div>in Camera::configure(), just before the call to PipelineHandler::configure().</div><div>If there is no requirement to call CameraConfiguration::validate(), would it</div><div>matter where the result gets set?</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>
Then the validate phase should be able to return the mode sensitivity of<br>
the configuration that is being validated.<br>
<br>
Doing that we could keep the configure calls using a const parameter for<br>
their config structures.<br>
<br>
> if (ret < 0) {<br>
> LOG(RPI, Error) << "IPA configuration failed!";<br>
> return -EPIPE;<br>
> }<br>
> <br>
> + /* Store the mode sensitivity for the application. */<br>
> + config->modeSensitivity = result.modeSensitivity;<br>
> +<br>
> /*<br>
> * Configure the H/V flip controls based on the combination of<br>
> * the sensor and user transform.<br>
> -- <br>
> 2.20.1<br>
><br>
</blockquote></div></div>