[libcamera-devel] [PATCH 2/7] pipeline: ipa: raspberrypi: Open the CamHelper on ipa::init()
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Mon Mar 22 22:06:32 CET 2021
Hi Naush,
Thank you for the patch.
On Wed, Mar 17, 2021 at 10:02:06AM +0000, Naushir Patuck wrote:
> Move the opening of the CamHelper from ipa::configure() to ipa::init().
> This allows the pipeline handler to get the sensor specific parameters
> in in pipeline_handler::match() where the ipa is initialised.
s/in in/in/
>
> Having the sensor parameters available earlier will allow selective
> use of the embedded data node in a future change.
>
> Signed-off-by: Naushir Patuck <naush at raspberrypi.com>
> ---
> include/libcamera/ipa/raspberrypi.mojom | 7 +--
> src/ipa/raspberrypi/raspberrypi.cpp | 60 +++++++++----------
> .../pipeline/raspberrypi/raspberrypi.cpp | 36 ++++++-----
> 3 files changed, 46 insertions(+), 57 deletions(-)
>
> diff --git a/include/libcamera/ipa/raspberrypi.mojom b/include/libcamera/ipa/raspberrypi.mojom
> index eb427697d349..a713c88eee19 100644
> --- a/include/libcamera/ipa/raspberrypi.mojom
> +++ b/include/libcamera/ipa/raspberrypi.mojom
> @@ -15,10 +15,6 @@ enum BufferMask {
> /* Size of the LS grid allocation. */
> const uint32 MaxLsGridSize = 0x8000;
>
> -enum ConfigOutputParameters {
> - ConfigSensorParams = 0x01,
> -};
> -
> struct SensorConfig {
> uint32 gainDelay;
> uint32 exposureDelay;
> @@ -41,7 +37,6 @@ struct ConfigInput {
>
> struct ConfigOutput {
> uint32 params;
Unless I'm mistaken, the params field isn't used anymore. Unless it's
used in a subsequent patch in the series, you can drop it.
> - SensorConfig sensorConfig;
> ControlList controls;
> };
>
> @@ -51,7 +46,7 @@ struct StartControls {
> };
>
> interface IPARPiInterface {
> - init(IPASettings settings) => (int32 ret);
> + init(IPASettings settings) => (int32 ret, SensorConfig sensorConfig);
> start(StartControls controls) => (StartControls result);
> stop();
>
> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp
> index 7904225a29fa..8a9eb92b39ec 100644
> --- a/src/ipa/raspberrypi/raspberrypi.cpp
> +++ b/src/ipa/raspberrypi/raspberrypi.cpp
> @@ -79,7 +79,7 @@ public:
> munmap(lsTable_, ipa::RPi::MaxLsGridSize);
> }
>
> - int init(const IPASettings &settings) override;
> + int init(const IPASettings &settings, ipa::RPi::SensorConfig *sensorConfig) override;
> void start(const ipa::RPi::StartControls &data,
> ipa::RPi::StartControls *result) override;
> void stop() override {}
> @@ -164,9 +164,35 @@ private:
> double maxFrameDuration_;
> };
>
> -int IPARPi::init(const IPASettings &settings)
> +int IPARPi::init(const IPASettings &settings, ipa::RPi::SensorConfig *sensorConfig)
> {
> tuningFile_ = settings.configurationFile;
> +
> + /*
> + * Load the "helper" for this sensor. This tells us all the device specific stuff
> + * that the kernel driver doesn't. We only do this the first time; we don't need
> + * to re-parse the metadata after a simple mode-switch for no reason.
> + */
> + helper_ = std::unique_ptr<RPiController::CamHelper>(RPiController::CamHelper::Create(settings.sensorModel));
Not something that needs to be done as a prerequisite, but would it make
sense to change the return type of RPiController::CamHelper::Create() to
std::unique_ptr<RPiController::CamHelper> ?
> + if (!helper_) {
> + LOG(IPARPI, Error) << "Could not create camera helper for "
> + << settings.sensorModel;
> + return -EINVAL;
> + }
> +
> + /*
> + * Pass out the sensor config to the pipeline handler in order
> + * to setup the staggered writer class.
> + */
> + int gainDelay, exposureDelay, vblankDelay, sensorMetadata;
> + helper_->GetDelays(exposureDelay, gainDelay, vblankDelay);
> + sensorMetadata = helper_->SensorEmbeddedDataPresent();
> +
> + sensorConfig->gainDelay = gainDelay;
> + sensorConfig->exposureDelay = exposureDelay;
> + sensorConfig->vblankDelay = vblankDelay;
> + sensorConfig->sensorMetadata = sensorMetadata;
> +
> return 0;
> }
>
> @@ -319,36 +345,6 @@ int IPARPi::configure(const CameraSensorInfo &sensorInfo,
> /* Setup a metadata ControlList to output metadata. */
> libcameraMetadata_ = ControlList(controls::controls);
>
> - /*
> - * Load the "helper" for this sensor. This tells us all the device specific stuff
> - * that the kernel driver doesn't. We only do this the first time; we don't need
> - * to re-parse the metadata after a simple mode-switch for no reason.
> - */
> - std::string cameraName(sensorInfo.model);
> - if (!helper_) {
> - helper_ = std::unique_ptr<RPiController::CamHelper>(RPiController::CamHelper::Create(cameraName));
> -
> - if (!helper_) {
> - LOG(IPARPI, Error) << "Could not create camera helper for "
> - << cameraName;
> - return -1;
> - }
> -
> - /*
> - * Pass out the sensor config to the pipeline handler in order
> - * to setup the staggered writer class.
> - */
> - int gainDelay, exposureDelay, vblankDelay, sensorMetadata;
> - helper_->GetDelays(exposureDelay, gainDelay, vblankDelay);
> - sensorMetadata = helper_->SensorEmbeddedDataPresent();
> -
> - result->params |= ipa::RPi::ConfigSensorParams;
> - result->sensorConfig.gainDelay = gainDelay;
> - result->sensorConfig.exposureDelay = exposureDelay;
> - result->sensorConfig.vblankDelay = vblankDelay;
> - result->sensorConfig.sensorMetadata = sensorMetadata;
> - }
> -
> /* Re-assemble camera mode using the sensor info. */
> setMode(sensorInfo);
>
> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> index 2c8ae31a28ad..ce1994186d66 100644
> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> @@ -146,7 +146,7 @@ public:
>
> void frameStarted(uint32_t sequence);
>
> - int loadIPA();
> + int loadIPA(ipa::RPi::SensorConfig *sensorConfig);
> int configureIPA(const CameraConfiguration *config);
>
> void statsMetadataComplete(uint32_t bufferId, const ControlList &controls);
> @@ -1030,11 +1030,24 @@ bool PipelineHandlerRPi::match(DeviceEnumerator *enumerator)
> if (data->sensor_->init())
> return false;
>
> - if (data->loadIPA()) {
> + ipa::RPi::SensorConfig sensorConfig;
> + if (data->loadIPA(&sensorConfig)) {
> LOG(RPI, Error) << "Failed to load a suitable IPA library";
> return false;
> }
>
> + /*
> + * Setup our delayed control writer with the sensor default
> + * gain and exposure delays. Mark VBLANK for priority write.
> + */
> + std::unordered_map<uint32_t, DelayedControls::ControlParams> params = {
> + { V4L2_CID_ANALOGUE_GAIN, { sensorConfig.gainDelay, false } },
> + { V4L2_CID_EXPOSURE, { sensorConfig.exposureDelay, false } },
> + { V4L2_CID_VBLANK, { sensorConfig.vblankDelay, true } }
> + };
> + data->delayedCtrls_ = std::make_unique<DelayedControls>(data->unicam_[Unicam::Image].dev(), params);
> + data->sensorMetadata_ = sensorConfig.sensorMetadata;
> +
As this code deals with RPiCameraData only, could you move it to
RPiCameraData::loadIPA() ? That way you won't have to prefix everything
with data->, and you'll avoid passing sensorConfig to loadIPA().
With these small changes,
Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> /* Register the controls that the Raspberry Pi IPA can handle. */
> data->controlInfo_ = RPi::Controls;
> /* Initialize the camera properties. */
> @@ -1214,7 +1227,7 @@ void RPiCameraData::frameStarted(uint32_t sequence)
> delayedCtrls_->applyControls(sequence);
> }
>
> -int RPiCameraData::loadIPA()
> +int RPiCameraData::loadIPA(ipa::RPi::SensorConfig *sensorConfig)
> {
> ipa_ = IPAManager::createIPA<ipa::RPi::IPAProxyRPi>(pipe_, 1, 1);
>
> @@ -1230,7 +1243,7 @@ int RPiCameraData::loadIPA()
> IPASettings settings(ipa_->configurationFile(sensor_->model() + ".json"),
> sensor_->model());
>
> - return ipa_->init(settings);
> + return ipa_->init(settings, sensorConfig);
> }
>
> int RPiCameraData::configureIPA(const CameraConfiguration *config)
> @@ -1293,21 +1306,6 @@ int RPiCameraData::configureIPA(const CameraConfiguration *config)
> return -EPIPE;
> }
>
> - if (result.params & ipa::RPi::ConfigSensorParams) {
> - /*
> - * Setup our delayed control writer with the sensor default
> - * gain and exposure delays. Mark VBLANK for priority write.
> - */
> - std::unordered_map<uint32_t, DelayedControls::ControlParams> params = {
> - { V4L2_CID_ANALOGUE_GAIN, { result.sensorConfig.gainDelay, false } },
> - { V4L2_CID_EXPOSURE, { result.sensorConfig.exposureDelay, false } },
> - { V4L2_CID_VBLANK, { result.sensorConfig.vblankDelay, true } }
> - };
> -
> - delayedCtrls_ = std::make_unique<DelayedControls>(unicam_[Unicam::Image].dev(), params);
> - sensorMetadata_ = result.sensorConfig.sensorMetadata;
> - }
> -
> if (!result.controls.empty()) {
> ControlList &ctrls = result.controls;
> unicam_[Unicam::Image].dev()->setControls(&ctrls);
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list