[libcamera-devel] [PATCH 4/4] ipa: rkisp1: add FrameDurationLimits control
Jacopo Mondi
jacopo at jmondi.org
Mon Nov 7 12:08:31 CET 2022
Hi Kieran
On Fri, Nov 04, 2022 at 12:37:34PM +0000, Kieran Bingham wrote:
> Quoting Jacopo Mondi via libcamera-devel (2022-11-04 10:50:04)
> > From: Nicholas Roth <nicholas at rothemail.net>
> >
> > Currently, the Android HAL does not work on rkisp1-based devices because
> > required FrameDurationLimits metadata is missing from the IPA
> > implementation.
> >
> > This change sets FrameDurationLimits for rkisp1 based on the existing
> > ipu3 implementation, using the sensor's reported range of vertical
> > blanking intervals with the minimum reported horizontal blanking
> > interval.
> >
> > Signed-off-by: Nicholas Roth <nicholas at rothemail.net>
> > ---
> > include/libcamera/ipa/rkisp1.mojom | 6 ++-
> > src/ipa/rkisp1/rkisp1.cpp | 57 +++++++++++++++++++++---
> > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 11 +++--
> > 3 files changed, 64 insertions(+), 10 deletions(-)
> >
> > diff --git a/include/libcamera/ipa/rkisp1.mojom b/include/libcamera/ipa/rkisp1.mojom
> > index 36bf291e8a51..1009e970a1b5 100644
> > --- a/include/libcamera/ipa/rkisp1.mojom
> > +++ b/include/libcamera/ipa/rkisp1.mojom
> > @@ -15,14 +15,16 @@ struct IPAConfigInfo {
> >
> > interface IPARkISP1Interface {
> > init(libcamera.IPASettings settings,
> > - uint32 hwRevision)
> > + uint32 hwRevision,
> > + libcamera.IPACameraSensorInfo sensorInfo,
> > + libcamera.ControlInfoMap sensorControls)
> > => (int32 ret, libcamera.ControlInfoMap ipaControls);
> > start() => (int32 ret);
> > stop();
> >
> > configure(IPAConfigInfo configInfo,
> > map<uint32, libcamera.IPAStream> streamConfig)
> > - => (int32 ret);
> > + => (int32 ret, libcamera.ControlInfoMap ipaControls);
> >
> > mapBuffers(array<libcamera.IPABuffer> buffers);
> > unmapBuffers(array<uint32> ids);
> > diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp
> > index e1ede62ead3a..6d1238d54f57 100644
> > --- a/src/ipa/rkisp1/rkisp1.cpp
> > +++ b/src/ipa/rkisp1/rkisp1.cpp
> > @@ -49,12 +49,15 @@ public:
> > IPARkISP1();
> >
> > int init(const IPASettings &settings, unsigned int hwRevision,
> > + const IPACameraSensorInfo &sensorInfo,
> > + const ControlInfoMap &sensorControls,
> > ControlInfoMap *ipaControls) override;
> > int start() override;
> > void stop() override;
> >
> > int configure(const IPAConfigInfo &ipaConfig,
> > - const std::map<uint32_t, IPAStream> &streamConfig) override;
> > + const std::map<uint32_t, IPAStream> &streamConfig,
> > + ControlInfoMap *ipaControls) override;
> > void mapBuffers(const std::vector<IPABuffer> &buffers) override;
> > void unmapBuffers(const std::vector<unsigned int> &ids) override;
> >
> > @@ -67,6 +70,9 @@ protected:
> > std::string logPrefix() const override;
> >
> > private:
> > + void updateControls(const IPACameraSensorInfo &sensorInfo,
> > + const ControlInfoMap &sensorControls,
> > + ControlInfoMap *ipaControls);
> > void setControls(unsigned int frame);
> >
> > std::map<unsigned int, FrameBuffer> buffers_;
> > @@ -114,6 +120,8 @@ std::string IPARkISP1::logPrefix() const
> > }
> >
> > int IPARkISP1::init(const IPASettings &settings, unsigned int hwRevision,
> > + const IPACameraSensorInfo &sensorInfo,
> > + const ControlInfoMap &sensorControls,
> > ControlInfoMap *ipaControls)
> > {
> > /* \todo Add support for other revisions */
> > @@ -179,9 +187,8 @@ int IPARkISP1::init(const IPASettings &settings, unsigned int hwRevision,
> > if (ret)
> > return ret;
> >
> > - /* Return the controls handled by the IPA. */
> > - ControlInfoMap::Map ctrlMap = rkisp1Controls;
> > - *ipaControls = ControlInfoMap(std::move(ctrlMap), controls::controls);
> > + /* Initialize controls. */
> > + updateControls(sensorInfo, sensorControls, ipaControls);
> >
> > return 0;
> > }
> > @@ -199,7 +206,8 @@ void IPARkISP1::stop()
> > }
> >
> > int IPARkISP1::configure(const IPAConfigInfo &ipaConfig,
> > - [[maybe_unused]] const std::map<uint32_t, IPAStream> &streamConfig)
> > + [[maybe_unused]] const std::map<uint32_t, IPAStream> &streamConfig,
> > + ControlInfoMap *ipaControls)
> > {
> > sensorControls_ = ipaConfig.sensorControls;
> >
> > @@ -229,6 +237,9 @@ int IPARkISP1::configure(const IPAConfigInfo &ipaConfig,
> > context_.configuration.sensor.size = info.outputSize;
> > context_.configuration.sensor.lineDuration = info.minLineLength * 1.0s / info.pixelRate;
> >
> > + /* Update the camera controls using the new sensor settings. */
> > + updateControls(info, sensorControls_, ipaControls);
> > +
> > /*
> > * When the AGC computes the new exposure values for a frame, it needs
> > * to know the limits for shutter speed and analogue gain.
> > @@ -329,6 +340,42 @@ void IPARkISP1::processStatsBuffer(const uint32_t frame, const uint32_t bufferId
> > metadataReady.emit(frame, metadata);
> > }
> >
> > +void IPARkISP1::updateControls(const IPACameraSensorInfo &sensorInfo,
> > + const ControlInfoMap &sensorControls,
> > + ControlInfoMap *ipaControls)
> > +{
> > + ControlInfoMap::Map ctrlMap = rkisp1Controls;
> > +
> > + /*
> > + * Compute the frame duration limits.
> > + *
> > + * The frame length is computed assuming a fixed line length combined
> > + * with the vertical frame sizes.
> > + */
>
> Does the CameraSensor class have enough information to do all this in a
> common location?
I presume so... Actually as this depends on the current configuration
CameraSensorInfo seems an ideal place..
However, it can safely be don on top, after we have aligned both
implementations ?
>
> Yakety Yak - but it seems like it might be nicer to have this code in
> CameraSensor class, and used by both IPU3 and RKISP1... (and therefore
> any other platform using CameraSensor class).
>
> Although - perhaps it's reliant upon the desired configuration too - but
> maybe it's still something that could live in libipa ?
>
> ControlInfo limits = libipa::frameDurationLimits(sensorControls,
> sensorInfo.outputSise);
>
> ?
>
> --
> Kieran
>
>
> > + 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);
> > + }
> > +
> > + ctrlMap[&controls::FrameDurationLimits] = ControlInfo(frameDurations[0],
> > + frameDurations[1],
> > + frameDurations[2]);
>
> Or
> ctrlMap[&controls::FrameDurationLimits] =
> libipa::frameDurationLimits(sensorControls,
> sensorInfo.outputSize);
>
> directly even?
>
> > +
> > + *ipaControls = ControlInfoMap(std::move(ctrlMap), controls::controls);
> > +}
> > +
> > void IPARkISP1::setControls(unsigned int frame)
> > {
> > /*
> > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > index 023d1fd4744f..8fbeed8ca800 100644
> > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > @@ -347,8 +347,13 @@ int RkISP1CameraData::loadIPA(unsigned int hwRevision)
> > ipaTuningFile = std::string(configFromEnv);
> > }
> >
> > - int ret = ipa_->init({ ipaTuningFile, sensor_->model() }, hwRevision,
> > - &controlInfo_);
> > + IPACameraSensorInfo sensorInfo{};
> > + int ret = sensor_->sensorInfo(&sensorInfo);
> > + if (ret)
> > + return ret;
> > +
> > + ret = ipa_->init({ ipaTuningFile, sensor_->model() }, hwRevision,
> > + sensorInfo, sensor_->controls(), &controlInfo_);
> > if (ret < 0) {
> > LOG(RkISP1, Error) << "IPA initialization failure";
> > return ret;
> > @@ -724,7 +729,7 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c)
> >
> > ipaConfig.sensorControls = data->sensor_->controls();
> >
> > - ret = data->ipa_->configure(ipaConfig, streamConfig);
> > + ret = data->ipa_->configure(ipaConfig, streamConfig, &data->controlInfo_);
> > if (ret) {
> > LOG(RkISP1, Error) << "failed configuring IPA (" << ret << ")";
> > return ret;
> > --
> > 2.38.1
> >
More information about the libcamera-devel
mailing list