[libcamera-devel] [PATCH v2 3/3] pipeline: ipa: raspberrypi: Pass controls to IPA on start
Naushir Patuck
naush at raspberrypi.com
Fri Dec 4 14:20:25 CET 2020
Hi Jacopo,
Thank you for your review comments.
On Fri, 4 Dec 2020 at 11:35, Jacopo Mondi <jacopo at jmondi.org> wrote:
> Hi Naush,
>
> On Thu, Nov 26, 2020 at 09:51:26AM +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 | 53 ++++++++++++-------
> > .../pipeline/raspberrypi/raspberrypi.cpp | 14 ++++-
> > 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)
> > };
> >
> > enum Operations {
> > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp
> b/src/ipa/raspberrypi/raspberrypi.cpp
> > index 7a07b477..c09b3d07 100644
> > --- a/src/ipa/raspberrypi/raspberrypi.cpp
> > +++ b/src/ipa/raspberrypi/raspberrypi.cpp
> > @@ -77,8 +77,7 @@ public:
> > }
> >
> > int init(const IPASettings &settings) override;
> > - int start([[maybe_unused]] const IPAOperationData &ipaConfig,
> > - [[maybe_unused]] IPAOperationData *result) override {
> return 0; }
> > + int start(const IPAOperationData &ipaConfig, IPAOperationData
> *result) override;
> > void stop() override {}
> >
> > void configure(const CameraSensorInfo &sensorInfo,
> > @@ -154,6 +153,35 @@ int IPARPi::init(const IPASettings &settings)
> > return 0;
> > }
> >
> > +int IPARPi::start(const IPAOperationData &ipaConfig, IPAOperationData
> *result)
> > +{
> > + RPiController::Metadata metadata;
> > +
> > + result->operation = 0;
>
> Do you need to assert (result) ?
> It might help catch development errors
>
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.
>
> > + if (ipaConfig.operation & RPi::IPA_CONFIG_STARTUP) {
> > + /* We have been given some controls to action before
> start. */
> > + queueRequest(ipaConfig.controls[0]);
>
> This seems to assume controls[0] is always populated...
>
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.
>
> > + }
> > +
> > + 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 +257,6 @@ void IPARPi::configure(const CameraSensorInfo
> &sensorInfo,
> > result->data.push_back(gainDelay);
> > result->data.push_back(exposureDelay);
> > result->data.push_back(sensorMetadata);
> > -
>
> Unrelated, but nothing big
>
> > result->operation |= RPi::IPA_CONFIG_STAGGERED_WRITE;
> > }
> >
> > @@ -285,11 +312,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 +319,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);
>
> I trust your judgment on this part that moves the mode switch to
> start() unconditionally
>
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.
Regards,
Naush
>
> > -
> > - /* 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;
> > }
> >
> > @@ -843,7 +858,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 29bcef07..3eb8c190 100644
> > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > @@ -748,7 +748,10 @@ int PipelineHandlerRPi::start(Camera *camera,
> [[maybe_unused]] ControlList *cont
> >
> > /* Start the IPA. */
> > IPAOperationData ipaConfig = {}, result = {};
> > - ipaConfig.controls.emplace_back(*controls);
> > + if (controls) {
> > + ipaConfig.operation = RPi::IPA_CONFIG_STARTUP;
> > + ipaConfig.controls.emplace_back(*controls);
> > + }
> > ret = data->ipa_->start(ipaConfig, &result);
> > if (ret) {
> > LOG(RPI, Error)
> > @@ -757,6 +760,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.
> > @@ -777,7 +788,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;
> > --
> > 2.25.1
> >
> > _______________________________________________
> > libcamera-devel mailing list
> > libcamera-devel at lists.libcamera.org
> > https://lists.libcamera.org/listinfo/libcamera-devel
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20201204/0a846c42/attachment-0001.htm>
More information about the libcamera-devel
mailing list