[libcamera-devel] [PATCH 2/7] pipeline: ipa: raspberrypi: Open the CamHelper on ipa::init()
Naushir Patuck
naush at raspberrypi.com
Mon Mar 22 22:22:04 CET 2021
Hi Laurent,
Thank you for your review feedback.
On Mon, 22 Mar 2021 at 21:07, Laurent Pinchart <
laurent.pinchart at ideasonboard.com> wrote:
> 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.
>
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?
Regards,
Naush
>
> 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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20210322/3c7c3401/attachment-0001.htm>
More information about the libcamera-devel
mailing list