<div dir="ltr"><div dir="ltr">Hi Laurent,</div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Tue, 8 Dec 2020 at 10:52, Laurent Pinchart <<a href="mailto:laurent.pinchart@ideasonboard.com">laurent.pinchart@ideasonboard.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 Tue, Dec 08, 2020 at 10:01:07AM +0000, Naushir Patuck wrote:<br>
> On Sat, 5 Dec 2020 at 21:30, Laurent Pinchart wrote:<br>
> > On Fri, Dec 04, 2020 at 03:31:21PM +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 | 54 ++++++++++++-------<br>
> > > .../pipeline/raspberrypi/raspberrypi.cpp | 13 ++++-<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>
> > Could you add a comma at the end of the line ?<br>
> ><br>
> > > };<br>
> > ><br>
> > > enum Operations {<br>
> > > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp<br>
> > > index b8c0f955..ff95580e 100644<br>
> > > --- a/src/ipa/raspberrypi/raspberrypi.cpp<br>
> > > +++ b/src/ipa/raspberrypi/raspberrypi.cpp<br>
> > > @@ -78,8 +78,7 @@ public:<br>
> > > }<br>
> > ><br>
> > > int init(const IPASettings &settings) override;<br>
> > > - int start([[maybe_unused]] const IPAOperationData &data,<br>
> > > - [[maybe_unused]] IPAOperationData *result) override { return 0; }<br>
> > > + int start(const IPAOperationData &data, IPAOperationData *result) override;<br>
> > > void stop() override {}<br>
> > ><br>
> > > void configure(const CameraSensorInfo &sensorInfo,<br>
> > > @@ -154,6 +153,36 @@ int IPARPi::init(const IPASettings &settings)<br>
> > > return 0;<br>
> > > }<br>
> > ><br>
> > > +int IPARPi::start(const IPAOperationData &data, IPAOperationData *result)<br>
> > > +{<br>
> > > + RPiController::Metadata metadata;<br>
> > > +<br>
> > > + ASSERT(result);<br>
> > > + result->operation = 0;<br>
> > > + if (data.operation & RPi::IPA_CONFIG_STARTUP) {<br>
> > > + /* We have been given some controls to action before start. */<br>
> > > + queueRequest(data.controls[0]);<br>
> ><br>
> > Not something to be addressed now, but do you foresee in the future a<br>
> > need for algorithms to handle initial controls differently than the<br>
> > controls passed through requests ? Requests are synchronized to frames,<br>
> > and I could imagine algorithms potentially relying on that sequencing.<br>
> > Reusing queueRequest() for initial controls would then interfere with<br>
> > that mechanism.<br>
> <br>
> This is an interesting question. There are certainly controls that can be<br>
> actioned outside of Requests and frames. Right now it does not matter too<br>
> much (if at all) for the Raspberry Pi implementation. To be honest, right<br>
> now I cannot think of concrete examples that would require this either. Do<br>
> you foresee a mechanism to set controls outside of the Request within<br>
> libcamera?<br>
<br>
Isn't this patch series adding such a mechanism ? ;-) I was just curious<br>
if you already saw a need to extend the controller API in this way in<br>
the future. I suppose we'll see when/if the need arises.<br></blockquote><div><br></div><div>Indeed it does :)</div><div>I was thinking more along the lines of after startup, but again, I don't have a particular use case or example in might right now, so no point trying to do this just yet.</div><div><br></div><div>Regards,</div><div>Naush</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 +258,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>
> > Unintentional change ?<br>
> ><br>
> > > result->operation |= RPi::IPA_CONFIG_STAGGERED_WRITE;<br>
> > > }<br>
> > ><br>
> > > @@ -285,11 +313,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 +320,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.emplace_back(ctrls);<br>
> > > result->operation |= RPi::IPA_CONFIG_SENSOR;<br>
> > > }<br>
> ><br>
> > Should we skip this at configure() time now that we do it at start()<br>
> > time ? Otherwise we'll set the controls twice.<br>
> ><br>
> > ><br>
> > > @@ -824,7 +840,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 bafd0b2d..bc7c56f8 100644<br>
> > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp<br>
> > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp<br>
> > > @@ -753,8 +753,10 @@ int PipelineHandlerRPi::start(Camera *camera, [[maybe_unused]] ControlList *cont<br>
> > ><br>
> > > /* Start the IPA. */<br>
> > > IPAOperationData ipaData = {}, result = {};<br>
> > > - if (controls)<br>
> > > + if (controls) {<br>
> > > + ipaData.operation = RPi::IPA_CONFIG_STARTUP;<br>
> > > ipaData.controls.emplace_back(*controls);<br>
> > > + }<br>
> > > ret = data->ipa_->start(ipaData, &result);<br>
> > > if (ret) {<br>
> > > LOG(RPI, Error)<br>
> > > @@ -763,6 +765,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>
> > > @@ -783,7 +793,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>
-- <br>
Regards,<br>
<br>
Laurent Pinchart<br>
</blockquote></div></div>