[libcamera-devel] [PATCH v3 3/3] pipeline: ipa: raspberrypi: Pass controls to IPA on start
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Tue Dec 8 11:52:05 CET 2020
Hi Naush,
On Tue, Dec 08, 2020 at 10:01:07AM +0000, Naushir Patuck wrote:
> On Sat, 5 Dec 2020 at 21:30, Laurent Pinchart wrote:
> > On Fri, Dec 04, 2020 at 03:31:21PM +0000, Naushir Patuck wrote:
> > > Forward any controls passed into the pipeline handler to the IPA.
> > > The IPA then sets up the Raspberry Pi controller with these settings
> > > appropriately, and passes back any V4L2 sensor controls that need
> > > to be applied.
> > >
> > > Signed-off-by: Naushir Patuck <naush at raspberrypi.com>
> > > Reviewed-by: David Plowman <david.plowman at raspberrypi.com>
> > > Tested-by: David Plowman <david.plowman at raspberrypi.com>
> > > ---
> > > include/libcamera/ipa/raspberrypi.h | 1 +
> > > src/ipa/raspberrypi/raspberrypi.cpp | 54 ++++++++++++-------
> > > .../pipeline/raspberrypi/raspberrypi.cpp | 13 ++++-
> > > 3 files changed, 47 insertions(+), 21 deletions(-)
> > >
> > > diff --git a/include/libcamera/ipa/raspberrypi.h b/include/libcamera/ipa/raspberrypi.h
> > > index 2b55dbfc..c91a14bd 100644
> > > --- a/include/libcamera/ipa/raspberrypi.h
> > > +++ b/include/libcamera/ipa/raspberrypi.h
> > > @@ -21,6 +21,7 @@ enum ConfigParameters {
> > > IPA_CONFIG_STAGGERED_WRITE = (1 << 1),
> > > IPA_CONFIG_SENSOR = (1 << 2),
> > > IPA_CONFIG_DROP_FRAMES = (1 << 3),
> > > + IPA_CONFIG_STARTUP = (1 << 4)
> >
> > Could you add a comma at the end of the line ?
> >
> > > };
> > >
> > > enum Operations {
> > > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp
> > > index b8c0f955..ff95580e 100644
> > > --- a/src/ipa/raspberrypi/raspberrypi.cpp
> > > +++ b/src/ipa/raspberrypi/raspberrypi.cpp
> > > @@ -78,8 +78,7 @@ public:
> > > }
> > >
> > > int init(const IPASettings &settings) override;
> > > - int start([[maybe_unused]] const IPAOperationData &data,
> > > - [[maybe_unused]] IPAOperationData *result) override { return 0; }
> > > + int start(const IPAOperationData &data, IPAOperationData *result) override;
> > > void stop() override {}
> > >
> > > void configure(const CameraSensorInfo &sensorInfo,
> > > @@ -154,6 +153,36 @@ int IPARPi::init(const IPASettings &settings)
> > > return 0;
> > > }
> > >
> > > +int IPARPi::start(const IPAOperationData &data, IPAOperationData *result)
> > > +{
> > > + RPiController::Metadata metadata;
> > > +
> > > + ASSERT(result);
> > > + result->operation = 0;
> > > + if (data.operation & RPi::IPA_CONFIG_STARTUP) {
> > > + /* We have been given some controls to action before start. */
> > > + queueRequest(data.controls[0]);
> >
> > Not something to be addressed now, but do you foresee in the future a
> > need for algorithms to handle initial controls differently than the
> > controls passed through requests ? Requests are synchronized to frames,
> > and I could imagine algorithms potentially relying on that sequencing.
> > Reusing queueRequest() for initial controls would then interfere with
> > that mechanism.
>
> This is an interesting question. There are certainly controls that can be
> actioned outside of Requests and frames. Right now it does not matter too
> much (if at all) for the Raspberry Pi implementation. To be honest, right
> now I cannot think of concrete examples that would require this either. Do
> you foresee a mechanism to set controls outside of the Request within
> libcamera?
Isn't this patch series adding such a mechanism ? ;-) I was just curious
if you already saw a need to extend the controller API in this way in
the future. I suppose we'll see when/if the need arises.
> > > + }
> > > +
> > > + controller_.SwitchMode(mode_, &metadata);
> > > +
> > > + /* SwitchMode may supply updated exposure/gain values to use. */
> > > + AgcStatus agcStatus;
> > > + agcStatus.shutter_time = 0.0;
> > > + agcStatus.analogue_gain = 0.0;
> > > +
> > > + /* SwitchMode may supply updated exposure/gain values to use. */
> > > + metadata.Get("agc.status", agcStatus);
> > > + if (agcStatus.shutter_time != 0.0 && agcStatus.analogue_gain != 0.0) {
> > > + ControlList ctrls(unicamCtrls_);
> > > + applyAGC(&agcStatus, ctrls);
> > > + result->controls.emplace_back(ctrls);
> > > + result->operation |= RPi::IPA_CONFIG_SENSOR;
> > > + }
> > > +
> > > + return 0;
> > > +}
> > > +
> > > void IPARPi::setMode(const CameraSensorInfo &sensorInfo)
> > > {
> > > mode_.bitdepth = sensorInfo.bitsPerPixel;
> > > @@ -229,7 +258,6 @@ void IPARPi::configure(const CameraSensorInfo &sensorInfo,
> > > result->data.push_back(gainDelay);
> > > result->data.push_back(exposureDelay);
> > > result->data.push_back(sensorMetadata);
> > > -
> >
> > Unintentional change ?
> >
> > > result->operation |= RPi::IPA_CONFIG_STAGGERED_WRITE;
> > > }
> > >
> > > @@ -285,11 +313,6 @@ void IPARPi::configure(const CameraSensorInfo &sensorInfo,
> > > result->data.push_back(dropFrame);
> > > result->operation |= RPi::IPA_CONFIG_DROP_FRAMES;
> > >
> > > - /* These zero values mean not program anything (unless overwritten). */
> > > - struct AgcStatus agcStatus;
> > > - agcStatus.shutter_time = 0.0;
> > > - agcStatus.analogue_gain = 0.0;
> > > -
> > > if (!controllerInit_) {
> > > /* Load the tuning file for this sensor. */
> > > controller_.Read(tuningFile_.c_str());
> > > @@ -297,20 +320,13 @@ void IPARPi::configure(const CameraSensorInfo &sensorInfo,
> > > controllerInit_ = true;
> > >
> > > /* Supply initial values for gain and exposure. */
> > > + ControlList ctrls(unicamCtrls_);
> > > + AgcStatus agcStatus;
> > > agcStatus.shutter_time = DefaultExposureTime;
> > > agcStatus.analogue_gain = DefaultAnalogueGain;
> > > - }
> > > -
> > > - RPiController::Metadata metadata;
> > > - controller_.SwitchMode(mode_, &metadata);
> > > -
> > > - /* SwitchMode may supply updated exposure/gain values to use. */
> > > - metadata.Get("agc.status", agcStatus);
> > > - if (agcStatus.shutter_time != 0.0 && agcStatus.analogue_gain != 0.0) {
> > > - ControlList ctrls(unicamCtrls_);
> > > applyAGC(&agcStatus, ctrls);
> > > - result->controls.push_back(ctrls);
> > >
> > > + result->controls.emplace_back(ctrls);
> > > result->operation |= RPi::IPA_CONFIG_SENSOR;
> > > }
> >
> > Should we skip this at configure() time now that we do it at start()
> > time ? Otherwise we'll set the controls twice.
> >
> > >
> > > @@ -824,7 +840,7 @@ void IPARPi::processStats(unsigned int bufferId)
> > >
> > > IPAOperationData op;
> > > op.operation = RPi::IPA_ACTION_V4L2_SET_STAGGERED;
> > > - op.controls.push_back(ctrls);
> > > + op.controls.emplace_back(ctrls);
> > > queueFrameAction.emit(0, op);
> > > }
> > > }
> > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > > index bafd0b2d..bc7c56f8 100644
> > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > > @@ -753,8 +753,10 @@ int PipelineHandlerRPi::start(Camera *camera, [[maybe_unused]] ControlList *cont
> > >
> > > /* Start the IPA. */
> > > IPAOperationData ipaData = {}, result = {};
> > > - if (controls)
> > > + if (controls) {
> > > + ipaData.operation = RPi::IPA_CONFIG_STARTUP;
> > > ipaData.controls.emplace_back(*controls);
> > > + }
> > > ret = data->ipa_->start(ipaData, &result);
> > > if (ret) {
> > > LOG(RPI, Error)
> > > @@ -763,6 +765,14 @@ int PipelineHandlerRPi::start(Camera *camera, [[maybe_unused]] ControlList *cont
> > > return ret;
> > > }
> > >
> > > + /* Apply any gain/exposure settings that the IPA may have passed back. */
> > > + ASSERT(data->staggeredCtrl_);
> > > + if (result.operation & RPi::IPA_CONFIG_SENSOR) {
> > > + const ControlList &ctrls = result.controls[0];
> > > + if (!data->staggeredCtrl_.set(ctrls))
> > > + LOG(RPI, Error) << "V4L2 staggered set failed";
> > > + }
> > > +
> > > /*
> > > * IPA configure may have changed the sensor flips - hence the bayer
> > > * order. Get the sensor format and set the ISP input now.
> > > @@ -783,7 +793,6 @@ int PipelineHandlerRPi::start(Camera *camera, [[maybe_unused]] ControlList *cont
> > > * starting. First check that the staggered ctrl has been initialised
> > > * by configure().
> > > */
> > > - ASSERT(data->staggeredCtrl_);
> > > data->staggeredCtrl_.reset();
> > > data->staggeredCtrl_.write();
> > > data->expectedSequence_ = 0;
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list