[libcamera-devel] [PATCH 2/7] pipeline: ipa: raspberrypi: Open the CamHelper on ipa::init()
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Tue Mar 23 14:02:38 CET 2021
Hi Naush,
On Mon, Mar 22, 2021 at 09:22:04PM +0000, Naushir Patuck wrote:
> On Mon, 22 Mar 2021 at 21:07, Laurent Pinchart wrote:
> > 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.
>
> This (and other bits) does get tidied up in the later commits for this
> series.
>
> > > - 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> ?
>
> Yes, that seems reasonable. I will make the change separately to this
> series.
>
> > > + 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().
>
> My original code did have this block in loadIPA(), but I ran into a small problem.
> DelayedControls() needs to have the device (unicam_[Unicam::Image] in this case)
> opened in order to validate the controls that are passed in via params. loadIPA()
> gets called before the devices are opened, so I had to move this code block
> here.
>
> I could have moved it into loadIPA() with unicam_[Unicam::Image] already opened,
> but then I would have to split the opening of unicam_[Unicam::Image] and
> unicam_[Unicam::Embedded] into two separate locations, which I thought might be
> undesirable. I am not completely against it, what are your thoughts?
It's a good point. Let's leave it as-is for now.
> > 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