| Message ID | 20210317100211.1067585-3-naush@raspberrypi.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series |
|
| Related | show |
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@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@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);
Hi Laurent, Thank you for your review feedback. On Mon, 22 Mar 2021 at 21:07, Laurent Pinchart < laurent.pinchart@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@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@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 >
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@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@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);
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; - 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)); + 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; + /* 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);
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. 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@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(-)