[libcamera-devel] [PATCH 1/2] libcamera: ipu3: Initialize controls in the IPA
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Tue Jul 27 05:17:59 CEST 2021
Hi Jacopo,
On Mon, Jul 26, 2021 at 10:26:41AM +0200, Jacopo Mondi wrote:
> On Sun, Jul 25, 2021 at 04:57:15AM +0300, Laurent Pinchart wrote:
> > On Fri, Jul 16, 2021 at 04:32:14PM +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 .
> > >
> > > Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> > > ---
> > > include/libcamera/ipa/ipu3.mojom | 8 ++-
> > > src/ipa/ipu3/ipu3.cpp | 71 ++++++++++++++++++--
> > > src/libcamera/pipeline/ipu3/ipu3.cpp | 96 ++++++++++++----------------
> > > 3 files changed, 116 insertions(+), 59 deletions(-)
> > >
> > > diff --git a/include/libcamera/ipa/ipu3.mojom b/include/libcamera/ipa/ipu3.mojom
> > > index 911a3a072464..eafefa8b7fe3 100644
> > > --- a/include/libcamera/ipa/ipu3.mojom
> > > +++ b/include/libcamera/ipa/ipu3.mojom
> > > @@ -30,6 +30,11 @@ struct IPU3Action {
> > > libcamera.ControlList controls;
> > > };
> > >
> > > +struct IPAInitInfo {
> > > + libcamera.IPACameraSensorInfo sensorInfo;
> > > + libcamera.ControlInfoMap sensorControls;
> > > +};
> > > +
> > > struct IPAConfigInfo {
> > > libcamera.IPACameraSensorInfo sensorInfo;
> > > map<uint32, libcamera.ControlInfoMap> entityControls;
> > > @@ -38,7 +43,8 @@ struct IPAConfigInfo {
> > > };
> > >
> > > interface IPAIPU3Interface {
> > > - init(libcamera.IPASettings settings) => (int32 ret);
> > > + init(IPAInitInfo initInfo)
> >
> > By dropping IPASettings, you drop the IPA configuration file. This isn't
> > used by the IPA module yet, but it should be in the future, so it would
> > be best to keep it. One option is to add libcamera.IPASettings as a
> > member of IPAInitInfo.
>
> Shouldn't this be done once anyone actually uses the configuration
> file ? Otherwise we should keep passing "" as part of the init()
> function argument list, which is cumbersome ?
We can do so as well, but as we know the need is there already... I'd
actually keep the settings parameter and add initInfo, instead of moving
settings in initInfo.
> > > + => (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..d3c69bc07bd0 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,8 @@ namespace ipa::ipu3 {
> > > class IPAIPU3 : public IPAIPU3Interface
> > > {
> > > public:
> > > - int init(const IPASettings &settings) override;
> > > + int init(const IPAInitInfo &initInfo, ControlInfoMap *ipaControls) override;
> > > +
> > > int start() override;
> > > void stop() override {}
> > >
> > > @@ -86,14 +89,74 @@ 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 IPAInitInfo &initInfo, ControlInfoMap *ipaControls)
> > > {
> > > - camHelper_ = CameraSensorHelperFactory::create(settings.sensorModel);
> > > + const IPACameraSensorInfo &sensorInfo = initInfo.sensorInfo;
> > > +
> > > + /* Initialize the camera sensor helper. */
> > > + camHelper_ = CameraSensorHelperFactory::create(sensorInfo.model);
> > > 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 "
> > > + << sensorInfo.model;
> > > return -ENODEV;
> > > }
> > >
> > > + /* Initialize Controls. */
> > > + const ControlInfoMap &sensorControls = initInfo.sensorControls;
> > > + 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 = std::move(controls);
> > > +
> > > return 0;
> > > }
> > >
> > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > > index 76c3bb3d8aa9..22df9c3650af 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);
> > > @@ -940,7 +942,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;
> > > @@ -952,58 +953,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.
> > > *
> > > @@ -1057,6 +1006,10 @@ int PipelineHandlerIPU3::initControls(IPU3CameraData *data)
> > >
> > > controls[&controls::ScalerCrop] = ControlInfo(minCrop, maxCrop, maxCrop);
> > >
> > > + /* Add the IPA registered controls to list of camera controls. */
> > > + for (const auto &ipaControl : data->ipaControls_)
> > > + controls[ipaControl.first] = ipaControl.second;
> >
> > It would be nice to use merge(), but ControlInfoMap inherits from
> > std::unordered_map<> with a private access specifier :-S Yet another
> > opportunity to improve the controls API.
> >
> > > +
> > > data->controlInfo_ = std::move(controls);
> > >
> > > return 0;
> > > @@ -1208,13 +1161,48 @@ int IPU3CameraData::loadIPA()
> > >
> > > ipa_->queueFrameAction.connect(this, &IPU3CameraData::queueFrameAction);
> > >
> > > + /*
> > > + * Pass the sensor info the the IPA to initialize controls.
> >
> > s/the the/to the/
> >
> > > + *
> > > + * \todo The limits of the registered controls depend on the current
> > > + * sensor configuration. Initialize the sensor using its resolution as
> > > + * its initial configuration and use it to compute the controls limits
> > > + * in the IPA.
> >
> > Could you rewrite this to explain what would need to be done to fix the
> > problem, instead of only describing the current implementation ? I'm
> > sure we understand it correctly now, but that may not be the case when
> > we'll tackle this todo item, or if someone less familiar with the issue
> > reads the comment.
>
> Not sure I know exactly what needs to be done, but I presume it boils
> down to find some sort of heuristic to identify a sensor default
> configuration to initialize controls with, like the Viewfinder
> configuration. Something like:
>
> *
> * \todo The limits of the registered controls depend on the current
> * sensor configuration. The sensor is currently initialized
> * using its resolution but a better heuristic should be
> * defined to identify the most opportune configuration to
> * initialize the Camera controls with. A possibility is to
> * use the configuration associated with one of the supported
> * Roles, such as the Viewfinder configuration.
> */
I'm not sure what the right solution is either (otherwise we would
implement it already :-)). I meant something like
/**
* \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;
> > > +
> > > + ipa::ipu3::IPAInitInfo initInfo{
> > > + sensorInfo,
> > > + sensor->controls(),
> > > + };
> > > + ipaControls_ = {};
> >
> > I think this line could be dropped, ipaControls_ is default-initialized
> > when IPU3CameraData is constructed, and loadIPA() is called once only.
>
> Correct!
>
> > Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> >
> > > + ret = ipa_->init(initInfo, &ipaControls_);
> > > if (ret) {
> > > LOG(IPU3, Error) << "Failed to initialise the IPU3 IPA";
> > > return ret;
> > > }
> > >
> > > + /*
> > > + * \todo Report the actual exposure time, use the default for the
> > > + * moment.
> > > + */
> > > + const auto exposureInfo = ipaControls_.find(&controls::ExposureTime);
> > > + if (exposureInfo == ipaControls_.end()) {
> > > + LOG(IPU3, Error) << "Exposure control not initializaed by the IPA";
> > > + return -EINVAL;
> > > + }
> > > + exposureTime_ = exposureInfo->second.def().get<int32_t>();
> > > +
> > > return 0;
> > > }
> > >
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list