<div dir="ltr"><div dir="ltr">Hi Laurent,<div><br></div><div>Thank you for your review feedb</div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Mon, 22 Mar 2021 at 20:57, 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 Wed, Mar 17, 2021 at 10:02:05AM +0000, Naushir Patuck wrote:<br>
> Pass the sensor model string to the IPA init() method through the<br>
> IPASettings structure.<br>
> <br>
> Signed-off-by: Naushir Patuck <<a href="mailto:naush@raspberrypi.com" target="_blank">naush@raspberrypi.com</a>><br>
> ---<br>
> include/libcamera/ipa/core.mojom | 8 ++++++++<br>
> src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 3 ++-<br>
> src/libcamera/pipeline/vimc/vimc.cpp | 2 +-<br>
> test/ipa/ipa_interface_test.cpp | 2 +-<br>
> 4 files changed, 12 insertions(+), 3 deletions(-)<br>
> <br>
> diff --git a/include/libcamera/ipa/core.mojom b/include/libcamera/ipa/core.mojom<br>
> index 5236a672461c..5363f1c5b48b 100644<br>
> --- a/include/libcamera/ipa/core.mojom<br>
> +++ b/include/libcamera/ipa/core.mojom<br>
> @@ -145,8 +145,16 @@ struct IPABuffer {<br>
> * This field may be an empty string if the IPA doesn't require a configuration<br>
> * file.<br>
> */<br>
> +<br>
> + /**<br>
> + * \var IPASettings::sensorModel<br>
> + * \brief The sensor model name<br>
> + *<br>
> + * Provides the sensor model name to the IPA.<br>
> + */<br>
> struct IPASettings {<br>
> string configurationFile;<br>
> + string sensorModel;<br>
<br>
This makes complete sense for all IPAs. I wonder if we should remove it<br>
from CameraSensorInfo then, but that can be done on top of the series.<br>
<br>
> };<br>
> <br>
> /**<br>
> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp<br>
> index 00fa62c972ed..2c8ae31a28ad 100644<br>
> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp<br>
> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp<br>
> @@ -1227,7 +1227,8 @@ int RPiCameraData::loadIPA()<br>
> ipa_->setIspControls.connect(this, &RPiCameraData::setIspControls);<br>
> ipa_->setDelayedControls.connect(this, &RPiCameraData::setDelayedControls);<br>
> <br>
> - IPASettings settings(ipa_->configurationFile(sensor_->model() + ".json"));<br>
> + IPASettings settings(ipa_->configurationFile(sensor_->model() + ".json"),<br>
> + sensor_->model());<br>
> <br>
> return ipa_->init(settings);<br>
> }<br>
> diff --git a/src/libcamera/pipeline/vimc/vimc.cpp b/src/libcamera/pipeline/vimc/vimc.cpp<br>
> index 57c65b021c46..2dfb63ecf2ef 100644<br>
> --- a/src/libcamera/pipeline/vimc/vimc.cpp<br>
> +++ b/src/libcamera/pipeline/vimc/vimc.cpp<br>
> @@ -425,7 +425,7 @@ bool PipelineHandlerVimc::match(DeviceEnumerator *enumerator)<br>
> data->ipa_ = IPAManager::createIPA<ipa::vimc::IPAProxyVimc>(this, 0, 0);<br>
> if (data->ipa_ != nullptr) {<br>
> std::string conf = data->ipa_->configurationFile("vimc.conf");<br>
> - data->ipa_->init(IPASettings{ conf });<br>
> + data->ipa_->init(IPASettings{ conf, data->sensor_->model() });<br>
> } else {<br>
> LOG(VIMC, Warning) << "no matching IPA found";<br>
> }<br>
> diff --git a/test/ipa/ipa_interface_test.cpp b/test/ipa/ipa_interface_test.cpp<br>
> index 4b88806f8f67..d6ca6f5137b0 100644<br>
> --- a/test/ipa/ipa_interface_test.cpp<br>
> +++ b/test/ipa/ipa_interface_test.cpp<br>
> @@ -104,7 +104,7 @@ protected:<br>
> <br>
> /* Test initialization of IPA module. */<br>
> std::string conf = ipa_->configurationFile("vimc.conf");<br>
> - int ret = ipa_->init(IPASettings{ conf });<br>
> + int ret = ipa_->init(IPASettings{ conf, "vimc" });<br>
> if (ret < 0) {<br>
> cerr << "IPA interface init() failed" << endl;<br>
> return TestFail;<br>
<br>
Could you please also update the ipu3 and rkisp1 pipeline handlers ?<br></blockquote><div><br></div><div>I did intend on modifying them, but they call init() in slightly different ways:</div><div><br></div>ipu3: ipa_->init(IPASettings{});<br>rkisp1: ipa_->init(hwRevision);</div><div class="gmail_quote"><br></div><div class="gmail_quote">rkisp1 does not use IPASettings at all, so I left it out, and ipu3 initialises an</div><div class="gmail_quote">empty IPASettings object. I could initialise ipu3 with the something like the</div><div class="gmail_quote">following:</div><div class="gmail_quote"><br></div><div class="gmail_quote">diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp<br>index 2ea13ec9e1b9..0e9b05f2b771 100644<br>--- a/src/libcamera/pipeline/ipu3/ipu3.cpp<br>+++ b/src/libcamera/pipeline/ipu3/ipu3.cpp<br>@@ -1144,7 +1144,8 @@ int IPU3CameraData::loadIPA()<br> <br> ipa_->queueFrameAction.connect(this, &IPU3CameraData::queueFrameAction);<br> <br>- ipa_->init(IPASettings{});<br>+ CameraSensor *sensor = cio2_.sensor();<br>+ ipa_->init(IPASettings{"", sensor->model()});<br></div><div class="gmail_quote"><br></div><div class="gmail_quote">Let me know what you think is appropriate.</div><div class="gmail_quote"><br></div><div class="gmail_quote">Regards,</div><div class="gmail_quote">Naush</div><div class="gmail_quote"> <br></div><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
-- <br>
Regards,<br>
<br>
Laurent Pinchart<br>
</blockquote></div></div>