| Message ID | 20210809152308.31947-5-jacopo@jmondi.org |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
Hi Jacopo, Thank you for the patch. On Mon, Aug 09, 2021 at 05:23:07PM +0200, Jacopo Mondi wrote: > All the IPU3 Camera controls are currently initialized by the pipeline > handler which initializes them using the camera sensor configuration and > platform specific requirements. > > However, some controls are better initialized by the IPA, which might, > in example, cap the exposure times and frame duration to the constraints > of its algorithms implementation. > > Also, moving forward, the IPA should register controls to report its > capabilities, in example the ability to enable/disable 3A algorithms on > request. > > Move the existing controls initialization to the IPA, by providing > the sensor configuration and its controls to the IPU3IPA::init() > function, which initializes controls and returns them to the pipeline > through an output parameter. > > The existing controls initialization has been copied verbatim from the > pipeline handler to the IPA, if not a for few line breaks adjustments > and the resulting Camera controls values are not changed . s/ .$/./ > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > Reviewed-by: Paul Elder <paul.elder@ideasonboard.com> > --- > include/libcamera/ipa/ipu3.mojom | 5 +- > src/ipa/ipu3/ipu3.cpp | 71 ++++++++++++++++++++- > src/libcamera/pipeline/ipu3/ipu3.cpp | 95 ++++++++++++---------------- > 3 files changed, 113 insertions(+), 58 deletions(-) > > diff --git a/include/libcamera/ipa/ipu3.mojom b/include/libcamera/ipa/ipu3.mojom > index 911a3a072464..d561c2244907 100644 > --- a/include/libcamera/ipa/ipu3.mojom > +++ b/include/libcamera/ipa/ipu3.mojom > @@ -38,7 +38,10 @@ struct IPAConfigInfo { > }; > > interface IPAIPU3Interface { > - init(libcamera.IPASettings settings) => (int32 ret); > + init(libcamera.IPASettings settings, > + libcamera.IPACameraSensorInfo sensorInfo, > + libcamera.ControlInfoMap sensorControls) > + => (int32 ret, libcamera.ControlInfoMap ipaControls); > start() => (int32 ret); > stop(); > > diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp > index 71698d36e50f..5e4b2bdc9ace 100644 > --- a/src/ipa/ipu3/ipu3.cpp > +++ b/src/ipa/ipu3/ipu3.cpp > @@ -5,8 +5,10 @@ > * ipu3.cpp - IPU3 Image Processing Algorithms > */ > > +#include <array> > #include <stdint.h> > #include <sys/mman.h> > +#include <utility> > > #include <linux/intel-ipu3.h> > #include <linux/v4l2-controls.h> > @@ -38,7 +40,11 @@ namespace ipa::ipu3 { > class IPAIPU3 : public IPAIPU3Interface > { > public: > - int init(const IPASettings &settings) override; > + int init(const IPASettings &settings, > + const IPACameraSensorInfo &sensorInfo, > + const ControlInfoMap &sensorControls, > + ControlInfoMap *ipaControls) override; > + > int start() override; > void stop() override {} > > @@ -86,14 +92,73 @@ private: > struct ipu3_uapi_grid_config bdsGrid_; > }; > > -int IPAIPU3::init(const IPASettings &settings) > +/** > + * Initialize the IPA module and its controls. > + * > + * This function receives the camera sensor information from the pipeline > + * handler, computes the limits of the controls it handles and returns > + * them in the \a ipaControls output parameter. > + */ > +int IPAIPU3::init(const IPASettings &settings, > + const IPACameraSensorInfo &sensorInfo, > + const ControlInfoMap &sensorControls, > + ControlInfoMap *ipaControls) > { > camHelper_ = CameraSensorHelperFactory::create(settings.sensorModel); > if (camHelper_ == nullptr) { > - LOG(IPAIPU3, Error) << "Failed to create camera sensor helper for " << settings.sensorModel; > + LOG(IPAIPU3, Error) << "Failed to create camera sensor helper for " > + << settings.sensorModel; While at it, this could become LOG(IPAIPU3, Error) << "Failed to create camera sensor helper for " << settings.sensorModel; > return -ENODEV; > } > > + /* Initialize Controls. */ > + ControlInfoMap::Map controls{}; > + > + /* > + * Compute exposure time limits. > + * > + * Initialize the control using the line length and pixel rate of the > + * current configuration converted to microseconds. Use the > + * V4L2_CID_EXPOSURE control to get exposure min, max and default and > + * convert it from lines to microseconds. > + */ > + double lineDuration = sensorInfo.lineLength / (sensorInfo.pixelRate / 1e6); > + const ControlInfo &v4l2Exposure = sensorControls.find(V4L2_CID_EXPOSURE)->second; > + int32_t minExposure = v4l2Exposure.min().get<int32_t>() * lineDuration; > + int32_t maxExposure = v4l2Exposure.max().get<int32_t>() * lineDuration; > + int32_t defExposure = v4l2Exposure.def().get<int32_t>() * lineDuration; > + controls[&controls::ExposureTime] = ControlInfo(minExposure, maxExposure, > + defExposure); > + > + /* > + * Compute the frame duration limits. > + * > + * The frame length is computed assuming a fixed line length combined > + * with the vertical frame sizes. > + */ > + const ControlInfo &v4l2HBlank = sensorControls.find(V4L2_CID_HBLANK)->second; > + uint32_t hblank = v4l2HBlank.def().get<int32_t>(); > + uint32_t lineLength = sensorInfo.outputSize.width + hblank; > + > + const ControlInfo &v4l2VBlank = sensorControls.find(V4L2_CID_VBLANK)->second; > + std::array<uint32_t, 3> frameHeights{ > + v4l2VBlank.min().get<int32_t>() + sensorInfo.outputSize.height, > + v4l2VBlank.max().get<int32_t>() + sensorInfo.outputSize.height, > + v4l2VBlank.def().get<int32_t>() + sensorInfo.outputSize.height, > + }; > + > + std::array<int64_t, 3> frameDurations; > + for (unsigned int i = 0; i < frameHeights.size(); ++i) { > + uint64_t frameSize = lineLength * frameHeights[i]; > + frameDurations[i] = frameSize / (sensorInfo.pixelRate / 1000000U); > + } > + > + controls[&controls::FrameDurationLimits] = ControlInfo(frameDurations[0], > + frameDurations[1], > + frameDurations[2]); > + > + *ipaControls = ControlInfoMap(std::move(controls), controls::controls); > + > return 0; > } > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp > index 9c23788e5231..a2d2c887590f 100644 > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp > @@ -88,6 +88,8 @@ public: > > std::queue<Request *> pendingRequests_; > > + ControlInfoMap ipaControls_; > + > private: > void queueFrameAction(unsigned int id, > const ipa::ipu3::IPU3Action &action); > @@ -954,7 +956,6 @@ int PipelineHandlerIPU3::initControls(IPU3CameraData *data) > return ret; > > ControlInfoMap::Map controls = IPU3Controls; > - const ControlInfoMap &sensorControls = sensor->controls(); > const std::vector<int32_t> &testPatternModes = sensor->testPatternModes(); > if (!testPatternModes.empty()) { > std::vector<ControlValue> values; > @@ -966,58 +967,6 @@ int PipelineHandlerIPU3::initControls(IPU3CameraData *data) > controls[&controls::draft::TestPatternMode] = ControlInfo(values); > } > > - /* > - * Compute exposure time limits. > - * > - * Initialize the control using the line length and pixel rate of the > - * current configuration converted to microseconds. Use the > - * V4L2_CID_EXPOSURE control to get exposure min, max and default and > - * convert it from lines to microseconds. > - */ > - double lineDuration = sensorInfo.lineLength > - / (sensorInfo.pixelRate / 1e6); > - const ControlInfo &v4l2Exposure = sensorControls.find(V4L2_CID_EXPOSURE)->second; > - int32_t minExposure = v4l2Exposure.min().get<int32_t>() * lineDuration; > - int32_t maxExposure = v4l2Exposure.max().get<int32_t>() * lineDuration; > - int32_t defExposure = v4l2Exposure.def().get<int32_t>() * lineDuration; > - > - /* > - * \todo Report the actual exposure time, use the default for the > - * moment. > - */ > - data->exposureTime_ = defExposure; > - > - controls[&controls::ExposureTime] = ControlInfo(minExposure, maxExposure, > - defExposure); > - > - /* > - * Compute the frame duration limits. > - * > - * The frame length is computed assuming a fixed line length combined > - * with the vertical frame sizes. > - */ > - const ControlInfo &v4l2HBlank = sensorControls.find(V4L2_CID_HBLANK)->second; > - uint32_t hblank = v4l2HBlank.def().get<int32_t>(); > - uint32_t lineLength = sensorInfo.outputSize.width + hblank; > - > - const ControlInfo &v4l2VBlank = sensorControls.find(V4L2_CID_VBLANK)->second; > - std::array<uint32_t, 3> frameHeights{ > - v4l2VBlank.min().get<int32_t>() + sensorInfo.outputSize.height, > - v4l2VBlank.max().get<int32_t>() + sensorInfo.outputSize.height, > - v4l2VBlank.def().get<int32_t>() + sensorInfo.outputSize.height, > - }; > - > - std::array<int64_t, 3> frameDurations; > - for (unsigned int i = 0; i < frameHeights.size(); ++i) { > - uint64_t frameSize = lineLength * frameHeights[i]; > - frameDurations[i] = frameSize / (sensorInfo.pixelRate / 1000000U); > - } > - > - controls[&controls::FrameDurationLimits] = > - ControlInfo(frameDurations[0], > - frameDurations[1], > - frameDurations[2]); > - > /* > * Compute the scaler crop limits. > * > @@ -1071,6 +1020,21 @@ int PipelineHandlerIPU3::initControls(IPU3CameraData *data) > > controls[&controls::ScalerCrop] = ControlInfo(minCrop, maxCrop, maxCrop); > > + /* > + * \todo Report the actual exposure time, use the default for the > + * moment. > + */ > + const auto exposureInfo = data->ipaControls_.find(&controls::ExposureTime); > + if (exposureInfo == data->ipaControls_.end()) { > + LOG(IPU3, Error) << "Exposure control not initialized by the IPA"; > + return -EINVAL; > + } > + data->exposureTime_ = exposureInfo->second.def().get<int32_t>(); > + > + /* Add the IPA registered controls to list of camera controls. */ > + for (const auto &ipaControl : data->ipaControls_) > + controls[ipaControl.first] = ipaControl.second; > + > data->controlInfo_ = ControlInfoMap(std::move(controls), > controls::controls); > > @@ -1223,8 +1187,31 @@ int IPU3CameraData::loadIPA() > > ipa_->queueFrameAction.connect(this, &IPU3CameraData::queueFrameAction); > > + /* > + * Pass the sensor info to the IPA to initialize controls. > + * > + * \todo Find a way to initialize IPA controls without basing their > + * limits on a particular sensor mode. We currently pass sensor > + * information corresponding to the largest sensor resolution, and the > + * IPA uses this to compute limits for supported controls. There's a > + * discrepancy between the need to compute IPA control limits at init > + * time, and the fact that those limits may depend on the sensor mode. > + * Research is required to find out to handle this issue. > + */ Nearly there, wrong indentation on the last line. Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > CameraSensor *sensor = cio2_.sensor(); > - int ret = ipa_->init(IPASettings{ "", sensor->model() }); > + V4L2SubdeviceFormat sensorFormat = {}; > + sensorFormat.size = sensor->resolution(); > + int ret = sensor->setFormat(&sensorFormat); > + if (ret) > + return ret; > + > + IPACameraSensorInfo sensorInfo{}; > + ret = sensor->sensorInfo(&sensorInfo); > + if (ret) > + return ret; > + > + ret = ipa_->init(IPASettings{ "", sensor->model() }, sensorInfo, > + sensor->controls(), &ipaControls_); > if (ret) { > LOG(IPU3, Error) << "Failed to initialise the IPU3 IPA"; > return ret;
diff --git a/include/libcamera/ipa/ipu3.mojom b/include/libcamera/ipa/ipu3.mojom index 911a3a072464..d561c2244907 100644 --- a/include/libcamera/ipa/ipu3.mojom +++ b/include/libcamera/ipa/ipu3.mojom @@ -38,7 +38,10 @@ struct IPAConfigInfo { }; interface IPAIPU3Interface { - init(libcamera.IPASettings settings) => (int32 ret); + init(libcamera.IPASettings settings, + libcamera.IPACameraSensorInfo sensorInfo, + libcamera.ControlInfoMap sensorControls) + => (int32 ret, libcamera.ControlInfoMap ipaControls); start() => (int32 ret); stop(); diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp index 71698d36e50f..5e4b2bdc9ace 100644 --- a/src/ipa/ipu3/ipu3.cpp +++ b/src/ipa/ipu3/ipu3.cpp @@ -5,8 +5,10 @@ * ipu3.cpp - IPU3 Image Processing Algorithms */ +#include <array> #include <stdint.h> #include <sys/mman.h> +#include <utility> #include <linux/intel-ipu3.h> #include <linux/v4l2-controls.h> @@ -38,7 +40,11 @@ namespace ipa::ipu3 { class IPAIPU3 : public IPAIPU3Interface { public: - int init(const IPASettings &settings) override; + int init(const IPASettings &settings, + const IPACameraSensorInfo &sensorInfo, + const ControlInfoMap &sensorControls, + ControlInfoMap *ipaControls) override; + int start() override; void stop() override {} @@ -86,14 +92,73 @@ private: struct ipu3_uapi_grid_config bdsGrid_; }; -int IPAIPU3::init(const IPASettings &settings) +/** + * Initialize the IPA module and its controls. + * + * This function receives the camera sensor information from the pipeline + * handler, computes the limits of the controls it handles and returns + * them in the \a ipaControls output parameter. + */ +int IPAIPU3::init(const IPASettings &settings, + const IPACameraSensorInfo &sensorInfo, + const ControlInfoMap &sensorControls, + ControlInfoMap *ipaControls) { camHelper_ = CameraSensorHelperFactory::create(settings.sensorModel); if (camHelper_ == nullptr) { - LOG(IPAIPU3, Error) << "Failed to create camera sensor helper for " << settings.sensorModel; + LOG(IPAIPU3, Error) << "Failed to create camera sensor helper for " + << settings.sensorModel; return -ENODEV; } + /* Initialize Controls. */ + ControlInfoMap::Map controls{}; + + /* + * Compute exposure time limits. + * + * Initialize the control using the line length and pixel rate of the + * current configuration converted to microseconds. Use the + * V4L2_CID_EXPOSURE control to get exposure min, max and default and + * convert it from lines to microseconds. + */ + double lineDuration = sensorInfo.lineLength / (sensorInfo.pixelRate / 1e6); + const ControlInfo &v4l2Exposure = sensorControls.find(V4L2_CID_EXPOSURE)->second; + int32_t minExposure = v4l2Exposure.min().get<int32_t>() * lineDuration; + int32_t maxExposure = v4l2Exposure.max().get<int32_t>() * lineDuration; + int32_t defExposure = v4l2Exposure.def().get<int32_t>() * lineDuration; + controls[&controls::ExposureTime] = ControlInfo(minExposure, maxExposure, + defExposure); + + /* + * Compute the frame duration limits. + * + * The frame length is computed assuming a fixed line length combined + * with the vertical frame sizes. + */ + const ControlInfo &v4l2HBlank = sensorControls.find(V4L2_CID_HBLANK)->second; + uint32_t hblank = v4l2HBlank.def().get<int32_t>(); + uint32_t lineLength = sensorInfo.outputSize.width + hblank; + + const ControlInfo &v4l2VBlank = sensorControls.find(V4L2_CID_VBLANK)->second; + std::array<uint32_t, 3> frameHeights{ + v4l2VBlank.min().get<int32_t>() + sensorInfo.outputSize.height, + v4l2VBlank.max().get<int32_t>() + sensorInfo.outputSize.height, + v4l2VBlank.def().get<int32_t>() + sensorInfo.outputSize.height, + }; + + std::array<int64_t, 3> frameDurations; + for (unsigned int i = 0; i < frameHeights.size(); ++i) { + uint64_t frameSize = lineLength * frameHeights[i]; + frameDurations[i] = frameSize / (sensorInfo.pixelRate / 1000000U); + } + + controls[&controls::FrameDurationLimits] = ControlInfo(frameDurations[0], + frameDurations[1], + frameDurations[2]); + + *ipaControls = ControlInfoMap(std::move(controls), controls::controls); + return 0; } diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp index 9c23788e5231..a2d2c887590f 100644 --- a/src/libcamera/pipeline/ipu3/ipu3.cpp +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp @@ -88,6 +88,8 @@ public: std::queue<Request *> pendingRequests_; + ControlInfoMap ipaControls_; + private: void queueFrameAction(unsigned int id, const ipa::ipu3::IPU3Action &action); @@ -954,7 +956,6 @@ int PipelineHandlerIPU3::initControls(IPU3CameraData *data) return ret; ControlInfoMap::Map controls = IPU3Controls; - const ControlInfoMap &sensorControls = sensor->controls(); const std::vector<int32_t> &testPatternModes = sensor->testPatternModes(); if (!testPatternModes.empty()) { std::vector<ControlValue> values; @@ -966,58 +967,6 @@ int PipelineHandlerIPU3::initControls(IPU3CameraData *data) controls[&controls::draft::TestPatternMode] = ControlInfo(values); } - /* - * Compute exposure time limits. - * - * Initialize the control using the line length and pixel rate of the - * current configuration converted to microseconds. Use the - * V4L2_CID_EXPOSURE control to get exposure min, max and default and - * convert it from lines to microseconds. - */ - double lineDuration = sensorInfo.lineLength - / (sensorInfo.pixelRate / 1e6); - const ControlInfo &v4l2Exposure = sensorControls.find(V4L2_CID_EXPOSURE)->second; - int32_t minExposure = v4l2Exposure.min().get<int32_t>() * lineDuration; - int32_t maxExposure = v4l2Exposure.max().get<int32_t>() * lineDuration; - int32_t defExposure = v4l2Exposure.def().get<int32_t>() * lineDuration; - - /* - * \todo Report the actual exposure time, use the default for the - * moment. - */ - data->exposureTime_ = defExposure; - - controls[&controls::ExposureTime] = ControlInfo(minExposure, maxExposure, - defExposure); - - /* - * Compute the frame duration limits. - * - * The frame length is computed assuming a fixed line length combined - * with the vertical frame sizes. - */ - const ControlInfo &v4l2HBlank = sensorControls.find(V4L2_CID_HBLANK)->second; - uint32_t hblank = v4l2HBlank.def().get<int32_t>(); - uint32_t lineLength = sensorInfo.outputSize.width + hblank; - - const ControlInfo &v4l2VBlank = sensorControls.find(V4L2_CID_VBLANK)->second; - std::array<uint32_t, 3> frameHeights{ - v4l2VBlank.min().get<int32_t>() + sensorInfo.outputSize.height, - v4l2VBlank.max().get<int32_t>() + sensorInfo.outputSize.height, - v4l2VBlank.def().get<int32_t>() + sensorInfo.outputSize.height, - }; - - std::array<int64_t, 3> frameDurations; - for (unsigned int i = 0; i < frameHeights.size(); ++i) { - uint64_t frameSize = lineLength * frameHeights[i]; - frameDurations[i] = frameSize / (sensorInfo.pixelRate / 1000000U); - } - - controls[&controls::FrameDurationLimits] = - ControlInfo(frameDurations[0], - frameDurations[1], - frameDurations[2]); - /* * Compute the scaler crop limits. * @@ -1071,6 +1020,21 @@ int PipelineHandlerIPU3::initControls(IPU3CameraData *data) controls[&controls::ScalerCrop] = ControlInfo(minCrop, maxCrop, maxCrop); + /* + * \todo Report the actual exposure time, use the default for the + * moment. + */ + const auto exposureInfo = data->ipaControls_.find(&controls::ExposureTime); + if (exposureInfo == data->ipaControls_.end()) { + LOG(IPU3, Error) << "Exposure control not initialized by the IPA"; + return -EINVAL; + } + data->exposureTime_ = exposureInfo->second.def().get<int32_t>(); + + /* Add the IPA registered controls to list of camera controls. */ + for (const auto &ipaControl : data->ipaControls_) + controls[ipaControl.first] = ipaControl.second; + data->controlInfo_ = ControlInfoMap(std::move(controls), controls::controls); @@ -1223,8 +1187,31 @@ int IPU3CameraData::loadIPA() ipa_->queueFrameAction.connect(this, &IPU3CameraData::queueFrameAction); + /* + * Pass the sensor info to the IPA to initialize controls. + * + * \todo Find a way to initialize IPA controls without basing their + * limits on a particular sensor mode. We currently pass sensor + * information corresponding to the largest sensor resolution, and the + * IPA uses this to compute limits for supported controls. There's a + * discrepancy between the need to compute IPA control limits at init + * time, and the fact that those limits may depend on the sensor mode. + * Research is required to find out to handle this issue. + */ CameraSensor *sensor = cio2_.sensor(); - int ret = ipa_->init(IPASettings{ "", sensor->model() }); + V4L2SubdeviceFormat sensorFormat = {}; + sensorFormat.size = sensor->resolution(); + int ret = sensor->setFormat(&sensorFormat); + if (ret) + return ret; + + IPACameraSensorInfo sensorInfo{}; + ret = sensor->sensorInfo(&sensorInfo); + if (ret) + return ret; + + ret = ipa_->init(IPASettings{ "", sensor->model() }, sensorInfo, + sensor->controls(), &ipaControls_); if (ret) { LOG(IPU3, Error) << "Failed to initialise the IPU3 IPA"; return ret;