<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>