[libcamera-devel] [PATCH v3 3/3] pipeline: ipa: raspberrypi: Pass controls to IPA on start
Naushir Patuck
naush at raspberrypi.com
Tue Dec 8 12:03:05 CET 2020
Hi Laurent,
On Tue, 8 Dec 2020 at 10:52, Laurent Pinchart <
laurent.pinchart at ideasonboard.com> wrote:
> 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.
>
Indeed it does :)
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.
Regards,
Naush
> > > > + }
> > > > +
> > > > + 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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20201208/6c2f11e5/attachment.htm>
More information about the libcamera-devel
mailing list