[libcamera-devel] [PATCH v3 3/3] pipeline: ipa: raspberrypi: Pass controls to IPA on start
Naushir Patuck
naush at raspberrypi.com
Tue Dec 8 11:01:07 CET 2020
Hi Laurent,
Thank you for your review comments.
On Sat, 5 Dec 2020 at 21:30, Laurent Pinchart <
laurent.pinchart at ideasonboard.com> wrote:
> Hi Naush,
>
> Thank you for the patch.
>
> 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?
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/3f26db89/attachment.htm>
More information about the libcamera-devel
mailing list