<div dir="ltr"><div dir="ltr">Hi Jacopo,<div><br></div><div>Thank you for your review comments.</div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Fri, 4 Dec 2020 at 11:35, Jacopo Mondi <<a href="mailto:jacopo@jmondi.org" target="_blank">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">Hi Naush,<br>
<br>
On Thu, Nov 26, 2020 at 09:51:26AM +0000, Naushir Patuck wrote:<br>
> Forward any controls passed into the pipeline handler to the IPA.<br>
> The IPA then sets up the Raspberry Pi controller with these settings<br>
> appropriately, and passes back any V4L2 sensor controls that need<br>
> to be applied.<br>
><br>
> Signed-off-by: Naushir Patuck <<a href="mailto:naush@raspberrypi.com" target="_blank">naush@raspberrypi.com</a>><br>
> Reviewed-by: David Plowman <<a href="mailto:david.plowman@raspberrypi.com" target="_blank">david.plowman@raspberrypi.com</a>><br>
> Tested-by: David Plowman <<a href="mailto:david.plowman@raspberrypi.com" target="_blank">david.plowman@raspberrypi.com</a>><br>
> ---<br>
>  include/libcamera/ipa/raspberrypi.h           |  1 +<br>
>  src/ipa/raspberrypi/raspberrypi.cpp           | 53 ++++++++++++-------<br>
>  .../pipeline/raspberrypi/raspberrypi.cpp      | 14 ++++-<br>
>  3 files changed, 47 insertions(+), 21 deletions(-)<br>
><br>
> diff --git a/include/libcamera/ipa/raspberrypi.h b/include/libcamera/ipa/raspberrypi.h<br>
> index 2b55dbfc..c91a14bd 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_STARTUP = (1 << 4)<br>
>  };<br>
><br>
>  enum Operations {<br>
> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp<br>
> index 7a07b477..c09b3d07 100644<br>
> --- a/src/ipa/raspberrypi/raspberrypi.cpp<br>
> +++ b/src/ipa/raspberrypi/raspberrypi.cpp<br>
> @@ -77,8 +77,7 @@ public:<br>
>       }<br>
><br>
>       int init(const IPASettings &settings) override;<br>
> -     int start([[maybe_unused]] const IPAOperationData &ipaConfig,<br>
> -               [[maybe_unused]] IPAOperationData *result) override { return 0; }<br>
> +     int start(const IPAOperationData &ipaConfig, IPAOperationData *result) override;<br>
>       void stop() override {}<br>
><br>
>       void configure(const CameraSensorInfo &sensorInfo,<br>
> @@ -154,6 +153,35 @@ int IPARPi::init(const IPASettings &settings)<br>
>       return 0;<br>
>  }<br>
><br>
> +int IPARPi::start(const IPAOperationData &ipaConfig, IPAOperationData *result)<br>
> +{<br>
> +     RPiController::Metadata metadata;<br>
> +<br>
> +     result->operation = 0;<br>
<br>
Do you need to assert (result) ?<br>
It might help catch development errors<br></blockquote><div><br></div><div>Given the only caller of this is the Raspberry Pi pipeline handler, result will always be valid.  However, always good to be extra safe, so I will add an assertion as suggested.</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>
> +     if (ipaConfig.operation & RPi::IPA_CONFIG_STARTUP) {<br>
> +             /* We have been given some controls to action before start. */<br>
> +             queueRequest(ipaConfig.controls[0]);<br>
<br>
This seems to assume controls[0] is always populated...<br></blockquote><div><br></div><div>The pipeline handler will only set RPi::IPA_CONFIG_STARTUP when a list of startup controls is available.  Thus, ipaConfig.controls[0] is guaranteed to be populated within the if() clause.<br></div><div> <br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
> +     }<br>
> +<br>
> +     controller_.SwitchMode(mode_, &metadata);<br>
> +<br>
> +     /* SwitchMode may supply updated exposure/gain values to use. */<br>
> +     AgcStatus agcStatus;<br>
> +     agcStatus.shutter_time = 0.0;<br>
> +     agcStatus.analogue_gain = 0.0;<br>
> +<br>
> +     /* SwitchMode may supply updated exposure/gain values to use. */<br>
> +     metadata.Get("agc.status", agcStatus);<br>
> +     if (agcStatus.shutter_time != 0.0 && agcStatus.analogue_gain != 0.0) {<br>
> +             ControlList ctrls(unicamCtrls_);<br>
> +             applyAGC(&agcStatus, ctrls);<br>
> +             result->controls.emplace_back(ctrls);<br>
> +             result->operation |= RPi::IPA_CONFIG_SENSOR;<br>
> +     }<br>
> +<br>
> +     return 0;<br>
> +}<br>
> +<br>
>  void IPARPi::setMode(const CameraSensorInfo &sensorInfo)<br>
>  {<br>
>       mode_.bitdepth = sensorInfo.bitsPerPixel;<br>
> @@ -229,7 +257,6 @@ void IPARPi::configure(const CameraSensorInfo &sensorInfo,<br>
>               result->data.push_back(gainDelay);<br>
>               result->data.push_back(exposureDelay);<br>
>               result->data.push_back(sensorMetadata);<br>
> -<br>
<br>
Unrelated, but nothing big<br>
<br>
>               result->operation |= RPi::IPA_CONFIG_STAGGERED_WRITE;<br>
>       }<br>
><br>
> @@ -285,11 +312,6 @@ void IPARPi::configure(const CameraSensorInfo &sensorInfo,<br>
>       result->data.push_back(dropFrame);<br>
>       result->operation |= RPi::IPA_CONFIG_DROP_FRAMES;<br>
><br>
> -     /* These zero values mean not program anything (unless overwritten). */<br>
> -     struct AgcStatus agcStatus;<br>
> -     agcStatus.shutter_time = 0.0;<br>
> -     agcStatus.analogue_gain = 0.0;<br>
> -<br>
>       if (!controllerInit_) {<br>
>               /* Load the tuning file for this sensor. */<br>
>               controller_.Read(tuningFile_.c_str());<br>
> @@ -297,20 +319,13 @@ void IPARPi::configure(const CameraSensorInfo &sensorInfo,<br>
>               controllerInit_ = true;<br>
><br>
>               /* Supply initial values for gain and exposure. */<br>
> +             ControlList ctrls(unicamCtrls_);<br>
> +             AgcStatus agcStatus;<br>
>               agcStatus.shutter_time = DefaultExposureTime;<br>
>               agcStatus.analogue_gain = DefaultAnalogueGain;<br>
> -     }<br>
> -<br>
> -     RPiController::Metadata metadata;<br>
> -     controller_.SwitchMode(mode_, &metadata);<br>
<br>
I trust your judgment on this part that moves the mode switch to<br>
start() unconditionally<br></blockquote><div><br></div><div>Yes, this is correct.  The rationale is that our controller_.SwitchMode() will return out the ISP and sensor configuration to apply before streaming, so must happen on every start.</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>
> -<br>
> -     /* SwitchMode may supply updated exposure/gain values to use. */<br>
> -     metadata.Get("agc.status", agcStatus);<br>
> -     if (agcStatus.shutter_time != 0.0 && agcStatus.analogue_gain != 0.0) {<br>
> -             ControlList ctrls(unicamCtrls_);<br>
>               applyAGC(&agcStatus, ctrls);<br>
> -             result->controls.push_back(ctrls);<br>
><br>
> +             result->controls.emplace_back(ctrls);<br>
>               result->operation |= RPi::IPA_CONFIG_SENSOR;<br>
>       }<br>
><br>
> @@ -843,7 +858,7 @@ void IPARPi::processStats(unsigned int bufferId)<br>
><br>
>               IPAOperationData op;<br>
>               op.operation = RPi::IPA_ACTION_V4L2_SET_STAGGERED;<br>
> -             op.controls.push_back(ctrls);<br>
> +             op.controls.emplace_back(ctrls);<br>
>               queueFrameAction.emit(0, op);<br>
>       }<br>
>  }<br>
> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp<br>
> index 29bcef07..3eb8c190 100644<br>
> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp<br>
> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp<br>
> @@ -748,7 +748,10 @@ int PipelineHandlerRPi::start(Camera *camera, [[maybe_unused]] ControlList *cont<br>
><br>
>       /* Start the IPA. */<br>
>       IPAOperationData ipaConfig = {}, result = {};<br>
> -     ipaConfig.controls.emplace_back(*controls);<br>
> +     if (controls) {<br>
> +             ipaConfig.operation = RPi::IPA_CONFIG_STARTUP;<br>
> +             ipaConfig.controls.emplace_back(*controls);<br>
> +     }<br>
>       ret = data->ipa_->start(ipaConfig, &result);<br>
>       if (ret) {<br>
>               LOG(RPI, Error)<br>
> @@ -757,6 +760,14 @@ int PipelineHandlerRPi::start(Camera *camera, [[maybe_unused]] ControlList *cont<br>
>               return ret;<br>
>       }<br>
><br>
> +     /* Apply any gain/exposure settings that the IPA may have passed back. */<br>
> +     ASSERT(data->staggeredCtrl_);<br>
> +     if (result.operation & RPi::IPA_CONFIG_SENSOR) {<br>
> +             const ControlList &ctrls = result.controls[0];<br>
> +             if (!data->staggeredCtrl_.set(ctrls))<br>
> +                     LOG(RPI, Error) << "V4L2 staggered set failed";<br>
> +     }<br>
> +<br>
>       /*<br>
>        * IPA configure may have changed the sensor flips - hence the bayer<br>
>        * order. Get the sensor format and set the ISP input now.<br>
> @@ -777,7 +788,6 @@ int PipelineHandlerRPi::start(Camera *camera, [[maybe_unused]] ControlList *cont<br>
>        * starting. First check that the staggered ctrl has been initialised<br>
>        * by configure().<br>
>        */<br>
> -     ASSERT(data->staggeredCtrl_);<br>
>       data->staggeredCtrl_.reset();<br>
>       data->staggeredCtrl_.write();<br>
>       data->expectedSequence_ = 0;<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>