<div dir="ltr"><div dir="ltr">Hi David,<div><br></div><div>Thank you for your comments.</div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Tue, 17 Nov 2020 at 16:36, David Plowman <<a href="mailto:david.plowman@raspberrypi.com">david.plowman@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>
Thanks for the patch! Only a couple of very minor things...<br>
<br>
On Thu, 12 Nov 2020 at 08:59, Naushir Patuck <<a href="mailto:naush@raspberrypi.com" target="_blank">naush@raspberrypi.com</a>> wrote:<br>
><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>
> ---<br>
> include/libcamera/ipa/raspberrypi.h | 1 +<br>
> src/ipa/raspberrypi/raspberrypi.cpp | 51 ++++++++++++-------<br>
> .../pipeline/raspberrypi/raspberrypi.cpp | 14 ++++-<br>
> 3 files changed, 46 insertions(+), 20 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..d4471f02 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>
Ah, I see we zero this here, answering some of my questions about the<br>
previous patch!<br></blockquote><div><br></div><div>Indeed, the result will always have this set, but perhaps better to be safe than sorry!</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>
> +<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.push_back(ctrls);<br>
<br>
I wonder if there's just a little bit more copying of controls going<br>
on here than is strictly necessary (maybe emplace_back?), but I agree<br>
that it's entirely inconsequential.<br></blockquote><div><br></div><div>Will change to emplace_back().</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_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>
> 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>
> - /* 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.push_back(ctrls);<br>
<br>
Same comment here. Though I wonder if maybe I wrote this originally,<br>
which would make it all my fault (most things are!).<br></blockquote><div><br></div><div>
Will change to emplace_back(). </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_SENSOR;<br>
> }<br>
><br>
> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp<br>
> index a8e4997a..6efe2403 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>
<br>
I guess this is the spot where I'd just want to convince myself<br>
nothing bad can happen if ipaConfig.operation is undefined but<br>
controls was NULL. I'm probably worrying over nothing...<br></blockquote><div><br></div><div>Indeed, over here ipaConfig should be explicitly intialised with = {}.</div><div><br></div><div>I will make the changes, and wait for further comments before posting the next version of the patch set.</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>
Those little things aside:<br>
<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>
Thanks and best regards<br>
David<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>