<div dir="ltr"><div dir="ltr">Hi Paul and Laurent,</div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Mon, 8 Mar 2021 at 08:47, 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 Paul,<br>
<br>
Thank you for the patch.<br>
<br>
On Mon, Mar 08, 2021 at 04:48:28PM +0900, Paul Elder wrote:<br>
> This is just a demo to show custom parameters to init() with the<br>
> raspberrypi IPA interface.<br>
> <br>
> Signed-off-by: Paul Elder <<a href="mailto:paul.elder@ideasonboard.com" target="_blank">paul.elder@ideasonboard.com</a>><br>
> ---<br>
> include/libcamera/ipa/raspberrypi.mojom | 5 ++-<br>
> src/ipa/raspberrypi/raspberrypi.cpp | 44 ++++++++++---------<br>
> .../pipeline/raspberrypi/raspberrypi.cpp | 10 +++--<br>
> 3 files changed, 32 insertions(+), 27 deletions(-)<br>
> <br>
> diff --git a/include/libcamera/ipa/raspberrypi.mojom b/include/libcamera/ipa/raspberrypi.mojom<br>
> index f733a2cd..99a62c02 100644<br>
> --- a/include/libcamera/ipa/raspberrypi.mojom<br>
> +++ b/include/libcamera/ipa/raspberrypi.mojom<br>
> @@ -51,7 +51,8 @@ struct StartControls {<br>
> };<br>
> <br>
> interface IPARPiInterface {<br>
> - init(IPASettings settings) => (int32 ret);<br>
> + init(IPASettings settings, string sensorName)<br>
> + => (int32 ret, bool metadataSupport);<br>
> start(StartControls controls) => (StartControls result);<br>
> stop();<br>
> <br>
> @@ -77,7 +78,7 @@ interface IPARPiInterface {<br>
> map<uint32, IPAStream> streamConfig,<br>
> map<uint32, ControlInfoMap> entityControls,<br>
> ConfigInput ipaConfig)<br>
> - => (ConfigOutput results, int32 ret);<br>
> + => (int32 ret, ConfigOutput results);<br>
<br>
I wonder if it would make sense to split those two changes. The change<br>
to configure() could be reviewed and merged already, and Naush could<br>
take this DEMO patch as an example to move data->sensorMetadata_<br>
handling to match() and IPA init() time.<br></blockquote><div><br></div><div>That is a reasonable approach. I got a few things on my list to clear off</div><div>before I get to this task, so separating it to allow you to get it merged</div><div>earlier would make sense.</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>
For the configure() part of this patch,<br>
<br>
Reviewed-by: Laurent Pinchart <<a href="mailto:laurent.pinchart@ideasonboard.com" target="_blank">laurent.pinchart@ideasonboard.com</a>><br>
<br>
You don't have to include an init() demo in v3 of the series (or just<br>
v2.1 of this patch actually), this is enough.<br>
<br>
> <br>
> /**<br>
> * \fn mapBuffers()<br>
> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp<br>
> index 6348d071..ac18dcbd 100644<br>
> --- a/src/ipa/raspberrypi/raspberrypi.cpp<br>
> +++ b/src/ipa/raspberrypi/raspberrypi.cpp<br>
> @@ -79,16 +79,17 @@ public:<br>
> munmap(lsTable_, ipa::RPi::MaxLsGridSize);<br>
> }<br>
> <br>
> - int init(const IPASettings &settings) override;<br>
> + int init(const IPASettings &settings, const std::string &sensorName,<br>
> + bool *metadataSupport) override;<br>
> void start(const ipa::RPi::StartControls &data,<br>
> ipa::RPi::StartControls *result) override;<br>
> void stop() override {}<br>
> <br>
> - void configure(const CameraSensorInfo &sensorInfo,<br>
> - const std::map<unsigned int, IPAStream> &streamConfig,<br>
> - const std::map<unsigned int, ControlInfoMap> &entityControls,<br>
> - const ipa::RPi::ConfigInput &data,<br>
> - ipa::RPi::ConfigOutput *response, int32_t *ret) override;<br>
> + int configure(const CameraSensorInfo &sensorInfo,<br>
> + const std::map<unsigned int, IPAStream> &streamConfig,<br>
> + const std::map<unsigned int, ControlInfoMap> &entityControls,<br>
> + const ipa::RPi::ConfigInput &data,<br>
> + ipa::RPi::ConfigOutput *response) 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>
> @@ -164,9 +165,14 @@ private:<br>
> double maxFrameDuration_;<br>
> };<br>
> <br>
> -int IPARPi::init(const IPASettings &settings)<br>
> +int IPARPi::init(const IPASettings &settings, const std::string &sensorName,<br>
> + bool *metadataSupport)<br>
> {<br>
> + LOG(IPARPI, Debug) << "sensor name is " << sensorName;<br>
> +<br>
> tuningFile_ = settings.configurationFile;<br>
> +<br>
> + *metadataSupport = true;<br>
> return 0;<br>
> }<br>
> <br>
> @@ -290,16 +296,15 @@ void IPARPi::setMode(const CameraSensorInfo &sensorInfo)<br>
> mode_.max_frame_length = sensorInfo.maxFrameLength;<br>
> }<br>
> <br>
> -void IPARPi::configure(const CameraSensorInfo &sensorInfo,<br>
> - [[maybe_unused]] const std::map<unsigned int, IPAStream> &streamConfig,<br>
> - const std::map<unsigned int, ControlInfoMap> &entityControls,<br>
> - const ipa::RPi::ConfigInput &ipaConfig,<br>
> - ipa::RPi::ConfigOutput *result, int32_t *ret)<br>
> +int IPARPi::configure(const CameraSensorInfo &sensorInfo,<br>
> + [[maybe_unused]] const std::map<unsigned int, IPAStream> &streamConfig,<br>
> + const std::map<unsigned int, ControlInfoMap> &entityControls,<br>
> + const ipa::RPi::ConfigInput &ipaConfig,<br>
> + ipa::RPi::ConfigOutput *result)<br>
> {<br>
> if (entityControls.size() != 2) {<br>
> LOG(IPARPI, Error) << "No ISP or sensor controls found.";<br>
> - *ret = -1;<br>
> - return;<br>
> + return -1;<br>
> }<br>
> <br>
> result->params = 0;<br>
> @@ -309,14 +314,12 @@ void IPARPi::configure(const CameraSensorInfo &sensorInfo,<br>
> <br>
> if (!validateSensorControls()) {<br>
> LOG(IPARPI, Error) << "Sensor control validation failed.";<br>
> - *ret = -1;<br>
> - return;<br>
> + return -1;<br>
> }<br>
> <br>
> if (!validateIspControls()) {<br>
> LOG(IPARPI, Error) << "ISP control validation failed.";<br>
> - *ret = -1;<br>
> - return;<br>
> + return -1;<br>
> }<br>
> <br>
> /* Setup a metadata ControlList to output metadata. */<br>
> @@ -334,8 +337,7 @@ void IPARPi::configure(const CameraSensorInfo &sensorInfo,<br>
> if (!helper_) {<br>
> LOG(IPARPI, Error) << "Could not create camera helper for "<br>
> << cameraName;<br>
> - *ret = -1;<br>
> - return;<br>
> + return -1;<br>
> }<br>
> <br>
> /*<br>
> @@ -403,7 +405,7 @@ void IPARPi::configure(const CameraSensorInfo &sensorInfo,<br>
> <br>
> lastMode_ = mode_;<br>
> <br>
> - *ret = 0;<br>
> + return 0;<br>
> }<br>
> <br>
> void IPARPi::mapBuffers(const std::vector<IPABuffer> &buffers)<br>
> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp<br>
> index db91f1b5..3ff0f1cd 100644<br>
> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp<br>
> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp<br>
> @@ -1194,7 +1194,10 @@ int RPiCameraData::loadIPA()<br>
> <br>
> IPASettings settings(ipa_->configurationFile(sensor_->model() + ".json"));<br>
> <br>
> - return ipa_->init(settings);<br>
> + bool metadataSupport;<br>
> + int ret = ipa_->init(settings, "sensor name", &metadataSupport);<br>
> + LOG(RPI, Debug) << "metadata support " << (metadataSupport ? "yes" : "no");<br>
> + return ret;<br>
> }<br>
> <br>
> int RPiCameraData::configureIPA(const CameraConfiguration *config)<br>
> @@ -1250,9 +1253,8 @@ int RPiCameraData::configureIPA(const CameraConfiguration *config)<br>
> /* Ready the IPA - it must know about the sensor resolution. */<br>
> ipa::RPi::ConfigOutput result;<br>
> <br>
> - ipa_->configure(sensorInfo_, streamConfig, entityControls, ipaConfig,<br>
> - &result, &ret);<br>
> -<br>
> + ret = ipa_->configure(sensorInfo_, streamConfig, entityControls, ipaConfig,<br>
> + &result);<br>
> if (ret < 0) {<br>
> LOG(RPI, Error) << "IPA configuration failed!";<br>
> return -EPIPE;<br>
<br>
-- <br>
Regards,<br>
<br>
Laurent Pinchart<br>
_______________________________________________<br>
libcamera-devel mailing list<br>
<a href="mailto:libcamera-devel@lists.libcamera.org" target="_blank">libcamera-devel@lists.libcamera.org</a><br>
<a href="https://lists.libcamera.org/listinfo/libcamera-devel" rel="noreferrer" target="_blank">https://lists.libcamera.org/listinfo/libcamera-devel</a><br>
</blockquote></div></div>