[libcamera-devel] [PATCH] pipeline: ipa: raspberrypi: Rationalise use of RPi::ConfigParameters
Jacopo Mondi
jacopo at jmondi.org
Mon Feb 1 11:53:49 CET 2021
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.
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 ?
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
More information about the libcamera-devel
mailing list