[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