[libcamera-devel] [PATCH] pipeline: ipa: raspberrypi: Rationalise use of RPi::ConfigParameters
Naushir Patuck
naush at raspberrypi.com
Mon Feb 1 13:41:42 CET 2021
Hi Jacopo,
Thank you for your review feedback.
On Mon, 1 Feb 2021 at 10:53, Jacopo Mondi <jacopo at jmondi.org> wrote:
> Hi Naush
>
> On Sat, Jan 30, 2021 at 11:16:51AM +0000, Naushir Patuck wrote:
> > Rename the enum values to indicate pipeline handler -> IPA actions
> > (IPA_CONFIG_*) and IPA -> pipeline handler return results (IPA_RESULT_*).
> > Additionally, provide more descriptive names for these values.
>
> This part is ok, make naming more clear
> Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>
>
> >
> > Fixup logic when handling IPA_RESULT_SENSOR_PARAMS where we must
> > overwrite the parameters if provided by IPA. In the current codebase,
> > this only happens once on startup, so there is no effective functional
> > difference. But this change allows the option for the IPA to request new
> > sensor parameters per-mode if required.
>
> This seems it's worth a separate patch, as it does not depends on
> renaming but it's rather a change in behaviour which it would probably
> better be separate.
>
Ack. I can separate these out into 2 patches. Will post an update shortly.
>
> If I got this right you refer to 'parameters' in the commit message
> here to indicate the controls delay could be overwritten at each
> configureIPA() am I right ? Out of curiosity, how does the sensor mode
> change impact on the control delay ?
>
In almost all cases it won't. However, I have sensors that will disable
embedded data in certain modes, but again, this is very rare.
Regards,
Naush
>
> Thanks
> j
>
>
> >
> > Signed-off-by: Naushir Patuck <naush at raspberrypi.com>
> > ---
> > include/libcamera/ipa/raspberrypi.h | 10 +++---
> > src/ipa/raspberrypi/raspberrypi.cpp | 18 +++++------
> > .../pipeline/raspberrypi/raspberrypi.cpp | 31 +++++++++----------
> > 3 files changed, 28 insertions(+), 31 deletions(-)
> >
> > diff --git a/include/libcamera/ipa/raspberrypi.h
> b/include/libcamera/ipa/raspberrypi.h
> > index 5a9433825d5a..970b9e931188 100644
> > --- a/include/libcamera/ipa/raspberrypi.h
> > +++ b/include/libcamera/ipa/raspberrypi.h
> > @@ -20,11 +20,11 @@ namespace RPi {
> >
> > enum ConfigParameters {
> > IPA_CONFIG_LS_TABLE = (1 << 0),
> > - IPA_CONFIG_STAGGERED_WRITE = (1 << 1),
> > - IPA_CONFIG_SENSOR = (1 << 2),
> > - IPA_CONFIG_DROP_FRAMES = (1 << 3),
> > - IPA_CONFIG_FAILED = (1 << 4),
> > - IPA_CONFIG_STARTUP = (1 << 5),
> > + IPA_CONFIG_STARTUP_CTRLS = (1 << 1),
> > + IPA_RESULT_CONFIG_FAILED = (1 << 2),
> > + IPA_RESULT_SENSOR_PARAMS = (1 << 3),
> > + IPA_RESULT_SENSOR_CTRLS = (1 << 4),
> > + IPA_RESULT_DROP_FRAMES = (1 << 5),
> > };
> >
> > enum Operations {
> > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp
> b/src/ipa/raspberrypi/raspberrypi.cpp
> > index 681ab9211b7c..fea1ea3957bb 100644
> > --- a/src/ipa/raspberrypi/raspberrypi.cpp
> > +++ b/src/ipa/raspberrypi/raspberrypi.cpp
> > @@ -170,7 +170,7 @@ int IPARPi::start(const IPAOperationData &data,
> IPAOperationData *result)
> >
> > ASSERT(result);
> > result->operation = 0;
> > - if (data.operation & RPi::IPA_CONFIG_STARTUP) {
> > + if (data.operation & RPi::IPA_CONFIG_STARTUP_CTRLS) {
> > /* We have been given some controls to action before
> start. */
> > queueRequest(data.controls[0]);
> > }
> > @@ -188,7 +188,7 @@ int IPARPi::start(const IPAOperationData &data,
> IPAOperationData *result)
> > ControlList ctrls(sensorCtrls_);
> > applyAGC(&agcStatus, ctrls);
> > result->controls.emplace_back(ctrls);
> > - result->operation |= RPi::IPA_CONFIG_SENSOR;
> > + result->operation |= RPi::IPA_RESULT_SENSOR_CTRLS;
> > }
> >
> > /*
> > @@ -236,7 +236,7 @@ int IPARPi::start(const IPAOperationData &data,
> IPAOperationData *result)
> > }
> >
> > result->data.push_back(dropFrame);
> > - result->operation |= RPi::IPA_CONFIG_DROP_FRAMES;
> > + result->operation |= RPi::IPA_RESULT_DROP_FRAMES;
> >
> > firstStart_ = false;
> >
> > @@ -289,7 +289,7 @@ void IPARPi::configure(const CameraSensorInfo
> &sensorInfo,
> > {
> > if (entityControls.size() != 2) {
> > LOG(IPARPI, Error) << "No ISP or sensor controls found.";
> > - result->operation = RPi::IPA_CONFIG_FAILED;
> > + result->operation = RPi::IPA_RESULT_CONFIG_FAILED;
> > return;
> > }
> >
> > @@ -300,13 +300,13 @@ void IPARPi::configure(const CameraSensorInfo
> &sensorInfo,
> >
> > if (!validateSensorControls()) {
> > LOG(IPARPI, Error) << "Sensor control validation failed.";
> > - result->operation = RPi::IPA_CONFIG_FAILED;
> > + result->operation = RPi::IPA_RESULT_CONFIG_FAILED;
> > return;
> > }
> >
> > if (!validateIspControls()) {
> > LOG(IPARPI, Error) << "ISP control validation failed.";
> > - result->operation = RPi::IPA_CONFIG_FAILED;
> > + result->operation = RPi::IPA_RESULT_CONFIG_FAILED;
> > return;
> > }
> >
> > @@ -325,7 +325,7 @@ void IPARPi::configure(const CameraSensorInfo
> &sensorInfo,
> > if (!helper_) {
> > LOG(IPARPI, Error) << "Could not create camera
> helper for "
> > << cameraName;
> > - result->operation = RPi::IPA_CONFIG_FAILED;
> > + result->operation = RPi::IPA_RESULT_CONFIG_FAILED;
> > return;
> > }
> >
> > @@ -342,7 +342,7 @@ void IPARPi::configure(const CameraSensorInfo
> &sensorInfo,
> > result->data.push_back(exposureDelay); /* For VBLANK ctrl
> */
> > result->data.push_back(sensorMetadata);
> >
> > - result->operation |= RPi::IPA_CONFIG_STAGGERED_WRITE;
> > + result->operation |= RPi::IPA_RESULT_SENSOR_PARAMS;
> > }
> >
> > /* Re-assemble camera mode using the sensor info. */
> > @@ -395,7 +395,7 @@ void IPARPi::configure(const CameraSensorInfo
> &sensorInfo,
> > applyAGC(&agcStatus, ctrls);
> >
> > result->controls.emplace_back(ctrls);
> > - result->operation |= RPi::IPA_CONFIG_SENSOR;
> > + result->operation |= RPi::IPA_RESULT_SENSOR_CTRLS;
> > }
> >
> > lastMode_ = mode_;
> > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > index 5ad12d99638f..c44cb437a596 100644
> > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > @@ -753,7 +753,7 @@ int PipelineHandlerRPi::start(Camera *camera,
> [[maybe_unused]] ControlList *cont
> > IPAOperationData ipaData = {};
> > IPAOperationData result = {};
> > if (controls) {
> > - ipaData.operation = RPi::IPA_CONFIG_STARTUP;
> > + ipaData.operation = RPi::IPA_CONFIG_STARTUP_CTRLS;
> > ipaData.controls.emplace_back(*controls);
> > }
> > ret = data->ipa_->start(ipaData, &result);
> > @@ -765,12 +765,12 @@ int PipelineHandlerRPi::start(Camera *camera,
> [[maybe_unused]] ControlList *cont
> > }
> >
> > /* Apply any gain/exposure settings that the IPA may have passed
> back. */
> > - if (result.operation & RPi::IPA_CONFIG_SENSOR) {
> > + if (result.operation & RPi::IPA_RESULT_SENSOR_CTRLS) {
> > ControlList &ctrls = result.controls[0];
> > data->unicam_[Unicam::Image].dev()->setControls(&ctrls);
> > }
> >
> > - if (result.operation & RPi::IPA_CONFIG_DROP_FRAMES) {
> > + if (result.operation & RPi::IPA_RESULT_DROP_FRAMES) {
> > /* Configure the number of dropped frames required on
> startup. */
> > data->dropFrameCount_ = result.data[0];
> > }
> > @@ -1213,31 +1213,28 @@ int RPiCameraData::configureIPA(const
> CameraConfiguration *config)
> > ipa_->configure(sensorInfo_, streamConfig, entityControls,
> ipaConfig,
> > &result);
> >
> > - if (result.operation & RPi::IPA_CONFIG_FAILED) {
> > + if (result.operation & RPi::IPA_RESULT_CONFIG_FAILED) {
> > LOG(RPI, Error) << "IPA configuration failed!";
> > return -EPIPE;
> > }
> >
> > unsigned int resultIdx = 0;
> > - if (result.operation & RPi::IPA_CONFIG_STAGGERED_WRITE) {
> > + if (result.operation & RPi::IPA_RESULT_SENSOR_PARAMS) {
> > /*
> > * Setup our delayed control writer with the sensor default
> > * gain and exposure delays.
> > */
> > - if (!delayedCtrls_) {
> > - std::unordered_map<uint32_t, unsigned int> delays
> = {
> > - { V4L2_CID_ANALOGUE_GAIN,
> result.data[resultIdx++] },
> > - { V4L2_CID_EXPOSURE,
> result.data[resultIdx++] },
> > - { V4L2_CID_VBLANK,
> result.data[resultIdx++] }
> > - };
> > -
> > - delayedCtrls_ =
> std::make_unique<DelayedControls>(unicam_[Unicam::Image].dev(), delays);
> > -
> > - sensorMetadata_ = result.data[resultIdx++];
> > - }
> > + std::unordered_map<uint32_t, unsigned int> delays = {
> > + { V4L2_CID_ANALOGUE_GAIN, result.data[resultIdx++]
> },
> > + { V4L2_CID_EXPOSURE, result.data[resultIdx++] },
> > + { V4L2_CID_VBLANK, result.data[resultIdx++] }
> > + };
> > +
> > + delayedCtrls_ =
> std::make_unique<DelayedControls>(unicam_[Unicam::Image].dev(), delays);
> > + sensorMetadata_ = result.data[resultIdx++];
> > }
> >
> > - if (result.operation & RPi::IPA_CONFIG_SENSOR) {
> > + if (result.operation & RPi::IPA_RESULT_SENSOR_CTRLS) {
> > ControlList &ctrls = result.controls[0];
> > unicam_[Unicam::Image].dev()->setControls(&ctrls);
> > }
> > --
> > 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/20210201/c3baa2c1/attachment-0001.htm>
More information about the libcamera-devel
mailing list