<div dir="ltr"><div dir="ltr">Hi Dave,<div><br></div><div>Thanks for the review.</div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Fri, 27 Nov 2020 at 14:21, Dave Stevenson <<a href="mailto:dave.stevenson@raspberrypi.com">dave.stevenson@raspberrypi.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>
On Fri, 27 Nov 2020 at 13:52, Naushir Patuck <<a href="mailto:naush@raspberrypi.com" target="_blank">naush@raspberrypi.com</a>> wrote:<br>
><br>
> HI Jacopo,<br>
><br>
> Thank you for the review.<br>
><br>
> On Fri, 27 Nov 2020 at 13:46, Jacopo Mondi <<a href="mailto:jacopo@jmondi.org" target="_blank">jacopo@jmondi.org</a>> wrote:<br>
>><br>
>> 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>
<br>
s/preset/present<br></blockquote><div><br></div><div>Ack. Will post an update shortly.</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>
>> > - 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>
><br>
><br>
> You are right, it is a bit pointless here. I will remove and submit an update after waiting a bit for any more feedback.<br>
><br>
> Regards,<br>
> Naush<br>
><br>
><br>
>><br>
>><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>
><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>