<div dir="ltr"><div dir="ltr">HI Jacopo,<div><br></div><div>Thank you for the review.</div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Fri, 27 Nov 2020 at 13:46, Jacopo Mondi <<a href="mailto:jacopo@jmondi.org">jacopo@jmondi.org</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">Hello Naush,<br>
<br>
On Fri, Nov 27, 2020 at 01:30:31PM +0000, Naushir Patuck wrote:<br>
> If the IPA fails during configuration, return an error flag to the<br>
> pipeline handler and fail the use case gracefully.<br>
><br>
> At preset, the IPA configuration can fail for the following reasons:<br>
> - The sensor is not recognised, and fails to open a CamHelper object.<br>
> - The pipeline handler did not pass in controls for the ISP and sensor.<br>
><br>
> Signed-off-by: Naushir Patuck <<a href="mailto:naush@raspberrypi.com" target="_blank">naush@raspberrypi.com</a>><br>
<br>
Looks good to me one minor nit apart. See below.<br>
<br>
Reviewed-by: Jacopo Mondi <<a href="mailto:jacopo@jmondi.org" target="_blank">jacopo@jmondi.org</a>><br>
<br>
Thanks<br>
  j<br>
<br>
> ---<br>
>  include/libcamera/ipa/raspberrypi.h  Â  Â  Â  Â  Â  Â  |  1 +<br>
>  src/ipa/raspberrypi/raspberrypi.cpp  Â  Â  Â  Â  Â  Â  | 16 ++++++++++++++--<br>
>  .../pipeline/raspberrypi/raspberrypi.cpp  Â  Â  Â  Â |  5 +++++<br>
>  3 files changed, 20 insertions(+), 2 deletions(-)<br>
><br>
> diff --git a/include/libcamera/ipa/raspberrypi.h b/include/libcamera/ipa/raspberrypi.h<br>
> index 2b55dbfc..86c97ffa 100644<br>
> --- a/include/libcamera/ipa/raspberrypi.h<br>
> +++ b/include/libcamera/ipa/raspberrypi.h<br>
> @@ -21,6 +21,7 @@ enum ConfigParameters {<br>
>  Â  Â  Â IPA_CONFIG_STAGGERED_WRITE = (1 << 1),<br>
>  Â  Â  Â IPA_CONFIG_SENSOR = (1 << 2),<br>
>  Â  Â  Â IPA_CONFIG_DROP_FRAMES = (1 << 3),<br>
> +  Â  Â IPA_CONFIG_FAILED = (1 << 4),<br>
>  };<br>
><br>
>  enum Operations {<br>
> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp<br>
> index 9853a343..7542dd26 100644<br>
> --- a/src/ipa/raspberrypi/raspberrypi.cpp<br>
> +++ b/src/ipa/raspberrypi/raspberrypi.cpp<br>
> @@ -197,8 +197,14 @@ void IPARPi::configure(const CameraSensorInfo &sensorInfo,<br>
>  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  const IPAOperationData &ipaConfig,<br>
>  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  IPAOperationData *result)<br>
>  {<br>
> -  Â  Â if (entityControls.empty())<br>
> +  Â  Â std::string cameraName(sensorInfo.model);<br>
> +<br>
> +  Â  Â if (entityControls.size() != 2) {<br>
> +  Â  Â  Â  Â  Â  Â LOG(IPARPI, Error) << "No ISP or sensor controls found for "<br>
> +  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  << cameraName;<br>
<br>
This will print the sensor name while the error is global to the<br>
camera, as the ISP controls might be missing as well.<br></blockquote><div><br></div><div>You are right, it is a bit pointless here.  I will remove and submit an update after waiting a bit for any more feedback.</div><div><br></div><div>Regards,</div><div>Naush</div><div><br></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>
> +  Â  Â  Â  Â  Â  Â result->operation = RPi::IPA_CONFIG_FAILED;<br>
>  Â  Â  Â  Â  Â  Â  Â return;<br>
> +  Â  Â }<br>
><br>
>  Â  Â  Â result->operation = 0;<br>
><br>
> @@ -213,10 +219,16 @@ void IPARPi::configure(const CameraSensorInfo &sensorInfo,<br>
>  Â  Â  Â  * that the kernel driver doesn't. We only do this the first time; we don't need<br>
>  Â  Â  Â  * to re-parse the metadata after a simple mode-switch for no reason.<br>
>  Â  Â  Â  */<br>
> -  Â  Â std::string cameraName(sensorInfo.model);<br>
>  Â  Â  Â if (!helper_) {<br>
>  Â  Â  Â  Â  Â  Â  Â helper_ = std::unique_ptr<RPiController::CamHelper>(RPiController::CamHelper::Create(cameraName));<br>
><br>
> +  Â  Â  Â  Â  Â  Â if (!helper_) {<br>
> +  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â LOG(IPARPI, Error) << "Could not create camera helper for "<br>
> +  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  << cameraName;<br>
> +  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â result->operation = RPi::IPA_CONFIG_FAILED;<br>
> +  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â return;<br>
> +  Â  Â  Â  Â  Â  Â }<br>
> +<br>
>  Â  Â  Â  Â  Â  Â  Â /*<br>
>  Â  Â  Â  Â  Â  Â  Â  * Pass out the sensor config to the pipeline handler in order<br>
>  Â  Â  Â  Â  Â  Â  Â  * to setup the staggered writer class.<br>
> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp<br>
> index 6fcdf557..76252806 100644<br>
> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp<br>
> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp<br>
> @@ -1194,6 +1194,11 @@ int RPiCameraData::configureIPA(const CameraConfiguration *config)<br>
>  Â  Â  Â ipa_->configure(sensorInfo_, streamConfig, entityControls, ipaConfig,<br>
>  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â &result);<br>
><br>
> +  Â  Â if (result.operation & RPi::IPA_CONFIG_FAILED) {<br>
> +  Â  Â  Â  Â  Â  Â LOG(RPI, Error) << "IPA configuration failed!";<br>
> +  Â  Â  Â  Â  Â  Â return -EPIPE;<br>
> +  Â  Â }<br>
> +<br>
>  Â  Â  Â unsigned int resultIdx = 0;<br>
>  Â  Â  Â if (result.operation & RPi::IPA_CONFIG_STAGGERED_WRITE) {<br>
>  Â  Â  Â  Â  Â  Â  Â /*<br>
> --<br>
> 2.25.1<br>
><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>