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