[libcamera-devel] [PATCH 3/3] pipeline: ipa: raspberrypi: Pass controls to IPA on start
Naushir Patuck
naush at raspberrypi.com
Wed Nov 18 11:27:40 CET 2020
Hi David,
Thank you for your comments.
On Tue, 17 Nov 2020 at 16:36, David Plowman <david.plowman at raspberrypi.com>
wrote:
> Hi Naush
>
> Thanks for the patch! Only a couple of very minor things...
>
> On Thu, 12 Nov 2020 at 08:59, Naushir Patuck <naush at raspberrypi.com>
> 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>
> > ---
> > include/libcamera/ipa/raspberrypi.h | 1 +
> > src/ipa/raspberrypi/raspberrypi.cpp | 51 ++++++++++++-------
> > .../pipeline/raspberrypi/raspberrypi.cpp | 14 ++++-
> > 3 files changed, 46 insertions(+), 20 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..d4471f02 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;
>
> Ah, I see we zero this here, answering some of my questions about the
> previous patch!
>
Indeed, the result will always have this set, but perhaps better to be safe
than sorry!
>
> > + if (ipaConfig.operation & RPi::IPA_CONFIG_STARTUP) {
> > + /* We have been given some controls to action before
> start. */
> > + queueRequest(ipaConfig.controls[0]);
> > + }
> > +
> > + 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.push_back(ctrls);
>
> I wonder if there's just a little bit more copying of controls going
> on here than is strictly necessary (maybe emplace_back?), but I agree
> that it's entirely inconsequential.
>
Will change to emplace_back().
>
> > + 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);
> > -
> > 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);
> > -
> > - /* 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.push_back(ctrls);
>
> Same comment here. Though I wonder if maybe I wrote this originally,
> which would make it all my fault (most things are!).
>
Will change to emplace_back().
>
> > result->operation |= RPi::IPA_CONFIG_SENSOR;
> > }
> >
> > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > index a8e4997a..6efe2403 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);
> > + }
>
> I guess this is the spot where I'd just want to convince myself
> nothing bad can happen if ipaConfig.operation is undefined but
> controls was NULL. I'm probably worrying over nothing...
>
Indeed, over here ipaConfig should be explicitly intialised with = {}.
I will make the changes, and wait for further comments before posting the
next version of the patch set.
Regards,
Naush
>
> Those little things aside:
>
> Reviewed-by: David Plowman <david.plowman at raspberrypi.com>
> Tested-by: David Plowman <david.plowman at raspberrypi.com>
>
> Thanks and best regards
> David
>
> > 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/20201118/1bf87238/attachment.htm>
More information about the libcamera-devel
mailing list